Project

General

Profile

Bug #15838

Fix non-blocking issues identified during ASP code review

Added by alant 12 months ago. Updated 10 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
09/26/2018
Due date:
% Done:

100%

Feature Branch:
bugfix/15838-asp-fix-non-blocking-issues
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Additional Software Packages

Description

My first batch of remarks:

  • I'd suggest you set `PERSISTENCE_SETUP_USERNAME = "tails-persistence-setup"` in tailslib/__init__.py and use it in the two occurrances where this string is currently used.

configuration-window.ui:

  • Maybe remove the install_label text, because it's overwritten in update_packages_list() anyway

tails-additional-software:

  • Why did you change the shebang from /usr/bin/env python3 to /usr/bin/python3? Usually using /usr/bin/env is preferred
  • Use logging.warning instead of deprecated logging.warn
  • _notify_failure(): * incomplete sentence in docstring: "The user has the option to edit the configuration of to view the system log" * second "details =" line uses .format() but the string doesn't contain any replacement fields
  • show_system_log(): the log file is not readable by amnesia, so gedit is not able to show it
  • apt_hook_pre(): you shouldn't use `assert` to check for conditions that could happen, because it is only evaluated if debug is True (see https://docs.python.org/3/reference/simple_stmts.html)
  • main(): What do we need the syslog handler for? Wouldn't it be better to just log to stderr (i.e. use logging.StreamHandler()) instead of logging to /dev/log? This way, if the program is run in a terminal, we get the output there directly, as would be expected, and else the output to stderr should go to the syslog anyway.

config.py:

  • there is a race condition between multiple calls of add_additional_packages and/or remove_additional_packages: if the config file is written between when the packages are read and when the packages are written, this change will be overwritten by the subsequent write. You could fix this by acquiring a file lock before reading the file, for example with fcntl.flock(f, fcntl.LOCK_EX), and only releasing the lock once the file was written (example: https://gitlab.com/segfault3/onionkit/blob/master/onionkit/util.py#L21).
    This way you could also get rid of the python3-atomicwrites package
  • it would be good to do the resetting of uid/gid in a `finally:` block, or even better, using a context manager
  • both add_additional_packages() and remove_additional_packages() are only ever called with search_new_persistence=True, so why is this argument even needed?
  • as a result of the above, get_additional_packages() and _write_config are also only called with search_new_persistence=True

org.boum.tails.additional-software.policy:

  • <message> is not translatable

persistence.py:

  • get_persistence_path(): only ever called with search_new_persistence=True, return_nonexistent=False
  • has_unlocked_persistence(): only called with search_new_persistence=True

Subtasks

Bug #15982: race condition between multiple calls of add_additional_packages and/or remove_additional_packagesDuplicate


Related issues

Related to Tails - Bug #16060: Improve ASP code: configuration window In Progress 10/16/2018
Related to Tails - Bug #16061: Improve ASP code: get rid of search_new_persistence argument Confirmed 10/16/2018
Related to Tails - Bug #16062: Improve ASP code: config.py Duplicate 10/16/2018
Related to Tails - Bug #16034: ASP: Fix race condition when writing to packages file In Progress 10/08/2018
Related to Tails - Bug #16110: Button to remove package in ASP GUI has a wrong label Resolved 11/08/2018
Blocks Tails - Feature #14598: Code review for Additional Software packages GUI Resolved 09/04/2017 04/15/2018

Associated revisions

Revision 8194bb55 (diff)
Added by alant 12 months ago

ASP: use logging.warning instead of deprecated logging.warn

Will-fix: #15838

Revision 4436405f (diff)
Added by alant 12 months ago

ASP: fix typo in docstring

Will-fix: #15838

Revision 005d0e51 (diff)
Added by alant 12 months ago

ASP: replace assert by raising an exception

is only evaluated if debug is True and should not be used to check for conditions.

Will-fix: #15838

Revision 10bfc0f2 (diff)
Added by alant 12 months ago

ASP: log to stderr

Will-Fix: #15838

Revision f7bd3edd (diff)
Added by alant 12 months ago

ASP: remove useless format

Will-fix: #15838

Revision 35b40834 (diff)
Added by alant 12 months ago

ASP: import new Tailslib

Will-fix: #15838

Revision 811de8af (diff)
Added by alant 12 months ago

ASP: import fixed tailslib

This should fix the build of the last fix for #15838.

Will-fix #15838.

Revision 8e3708cf (diff)
Added by bertagaz 11 months ago

ASP: Fix regression introduced in 10bfc0f.

StreamHandler() is part of the logging module, and not logging.handlers.

Refs: #15838

Revision b04b057d (diff)
Added by alant 10 months ago

ASP: use logging.warning instead of deprecated logging.warn

Will-fix: #15838

Revision 49739777 (diff)
Added by alant 10 months ago

ASP: fix typo in docstring

Will-fix: #15838

Revision 106c5265 (diff)
Added by alant 10 months ago

ASP: replace assert by raising an exception

assert is only evaluated if debug is True and should not be used to check
for conditions.

Will-fix: #15838

Revision 6d6880fa (diff)
Added by alant 10 months ago

ASP: log to stderr

Will-Fix: #15838

Revision 1e91cca2 (diff)
Added by alant 10 months ago

ASP: remove useless format

Will-fix: #15838

Revision 88752590
Added by intrigeri 10 months ago

Merge remote-tracking branch 'origin/bugfix/15838-asp-fix-non-blocking-issues' into stable (Fix-committed: #15983)

Refs: #15838

Revision 580f1eec
Added by intrigeri 10 months ago

Merge remote-tracking branch 'origin/bugfix/15838-asp-fix-non-blocking-issues' into stable (Closes: #15838)

Note: using "Closes" as the issue #15838 was about has been fixed in 3.10.1
already and this branch merely rewraps lines.

History

#2 Updated by alant 12 months ago

  • Related to Feature #14598: Code review for Additional Software packages GUI added

#3 Updated by alant 12 months ago

  • Assignee changed from alant to segfault
  • % Done changed from 0 to 30
  • QA Check set to Info Needed
  • I'd suggest you set `PERSISTENCE_SETUP_USERNAME = "tails-persistence-setup"` in tailslib/__init__.py and use it in the two occurrances where this string is currently used.

Done

configuration-window.ui:

  • Maybe remove the install_label text, because it's overwritten in update_packages_list() anyway

That would make the UI file less understandable for no benefit that I can see.

tails-additional-software:

  • Why did you change the shebang from /usr/bin/env python3 to /usr/bin/python3? Usually using /usr/bin/env is preferred

Done

  • Use logging.warning instead of deprecated logging.warn

Done

  • _notify_failure():
  • incomplete sentence in docstring: "The user has the option to edit the configuration of to view the system log"

Fixed

  • second "details =" line uses .format() but the string doesn't contain any replacement fields

Fixed

Fixed

  • main(): What do we need the syslog handler for? Wouldn't it be better to just log to stderr (i.e. use logging.StreamHandler()) instead of logging to /dev/log? This way, if the program is run in a terminal, we get the output there directly, as would be expected, and else the output to stderr should go to the syslog anyway.

Fixed

config.py:

Renamed to additionalsoftware.py so that we don't have an empty package.

  • there is a race condition between multiple calls of add_additional_packages and/or remove_additional_packages: if the config file is written between when the packages are read and when the packages are written, this change will be overwritten by the subsequent write. You could fix this by acquiring a file lock before reading the file, for example with fcntl.flock(f, fcntl.LOCK_EX), and only releasing the lock once the file was written (example: https://gitlab.com/segfault3/onionkit/blob/master/onionkit/util.py#L21).
    This way you could also get rid of the python3-atomicwrites package

As far as I understand, such lock are released on close, so I don't see how it would actually lock the config file (see https://linux.die.net/man/2/flock).
The way the code works now, the file is opened for reading, then closed, then opened again for writing, closed, and chmod.
We don't even have any guarantee we actually write the the same path we read, because get_packages_list_path is called again each time.
Am I wrong?
Do you think we should ensure the file is only opened once? Or at least that it is the same file the gets written that the one we read?

  • it would be good to do the resetting of uid/gid in a `finally:` block, or even better, using a context manager

I added a try...finally block. I don't see why a context manager used only once would be better.

  • both add_additional_packages() and remove_additional_packages() are only ever called with search_new_persistence=True, so why is this argument even needed?

You're right, it was meant for providing the same beahavior as before when I added the argument, but it's now useless for these functions.

  • as a result of the above, get_additional_packages() and _write_config are also only called with search_new_persistence=True

No, see config/chroot_local-includes/usr/local/sbin/tails-additional-software:516:

packages = get_additional_packages()

persistence.py:

  • get_persistence_path(): only ever called with search_new_persistence=True, return_nonexistent=False

No, see above note on search_new_persistence.

  • has_unlocked_persistence(): only called with search_new_persistence=True

Right.

I'm not sure wether it's better to remove the argument for the function where it's always called with True or to keep it so that they all behave the same way. I'm tempted to say the 2nd is more readeable, but that may be lazyness speaking. What do you think?

#4 Updated by alant 12 months ago

  • Status changed from Confirmed to In Progress

#5 Updated by alant 12 months ago

  • Feature Branch set to feature/14594-asp-gui

#6 Updated by alant 12 months ago

  • Blocked by Bug #15846: devel and branches based on devel FTBFS: The following packages have unmet dependencies added

#7 Updated by intrigeri 12 months ago

  • Blocked by deleted (Bug #15846: devel and branches based on devel FTBFS: The following packages have unmet dependencies)

#8 Updated by intrigeri 12 months ago

Blocked by Bug #15846: devel and branches based on devel FTBFS: The following packages have unmet dependencies added

That's incorrect: #15846 only affects devel while these fixes shall be developed on top of testing, until 3.9 is out, and then on top of stable.

#9 Updated by segfault 12 months ago

  • Assignee changed from segfault to alant
  • QA Check changed from Info Needed to Dev Needed

alant wrote:

configuration-window.ui:

  • Maybe remove the install_label text, because it's overwritten in update_packages_list() anyway

That would make the UI file less understandable for no benefit that I can see.

I see these disadvantages of keeping the string:
  • Can easily lead to confusion when someone wants to change that string
  • Will lead to unnecessary work for translators in case the string is changed in only one place

config.py:

Renamed to additionalsoftware.py so that we don't have an empty package.

  • there is a race condition between multiple calls of add_additional_packages and/or remove_additional_packages: if the config file is written between when the packages are read and when the packages are written, this change will be overwritten by the subsequent write. You could fix this by acquiring a file lock before reading the file, for example with fcntl.flock(f, fcntl.LOCK_EX), and only releasing the lock once the file was written (example: https://gitlab.com/segfault3/onionkit/blob/master/onionkit/util.py#L21).
    This way you could also get rid of the python3-atomicwrites package

As far as I understand, such lock are released on close, so I don't see how it would actually lock the config file (see https://linux.die.net/man/2/flock).
The way the code works now, the file is opened for reading, then closed, then opened again for writing, closed, and chmod.
We don't even have any guarantee we actually write the the same path we read, because get_packages_list_path is called again each time.
Am I wrong?
Do you think we should ensure the file is only opened once?

Yes, that's what I meant and what the flock can be used for.

  • it would be good to do the resetting of uid/gid in a `finally:` block, or even better, using a context manager

I added a try...finally block. I don't see why a context manager used only once would be better.

I generally prefer them for readability, but I'm also OK with the way you did it.

  • both add_additional_packages() and remove_additional_packages() are only ever called with search_new_persistence=True, so why is this argument even needed?

You're right, it was meant for providing the same beahavior as before when I added the argument, but it's now useless for these functions.

  • as a result of the above, get_additional_packages() and _write_config are also only called with search_new_persistence=True

No, see config/chroot_local-includes/usr/local/sbin/tails-additional-software:516:

packages = get_additional_packages()

I'm not sure wether it's better to remove the argument for the function where it's always called with True or to keep it so that they all behave the same way. I'm tempted to say the 2nd is more readeable, but that may be lazyness speaking. What do you think?

I think it would make the code more readable if you would just remove this argument from all functions, and always check if there is a new persistence. Even in this one line you quoted above, it only does not check for a new persistence because you expect that there is none anyway, because it's called right after boot, right? So it wouldn't be an issue if it would still check for a (non-existing) new persistence.

#10 Updated by segfault 12 months ago

intrigeri wrote:

Blocked by Bug #15846: devel and branches based on devel FTBFS: The following packages have unmet dependencies added

That's incorrect: #15846 only affects devel while these fixes shall be developed on top of testing, until 3.9 is out, and then on top of stable.

testing (and feature/14594-asp-gui) also FTBFS because linux-image-4.17.0-2-amd64 was removed from sid

#11 Updated by intrigeri 12 months ago

testing (and feature/14594-asp-gui) also FTBFS because linux-image-4.17.0-2-amd64 was removed from sid

Wrt. testing, this is surprising given that branch uses a frozen set of APT snapshots that has that package. So far no build has failed on Jenkins but I've just started one to confirm my hunch.
Wrt. feature/14594-asp-gui, for some reason it has devel has its base branch so it's of course affected by #15846, but that's a problem on the branch itself, which has to be fixed regardless of #15846.

#12 Updated by intrigeri 12 months ago

testing (and feature/14594-asp-gui) also FTBFS because linux-image-4.17.0-2-amd64 was removed from sid

Wrt. testing, this is surprising given that branch uses a frozen set of APT snapshots that has that package. So far no build has failed on Jenkins but I've just started one to confirm my hunch.

Confirmed, the testing branch builds just fine.

#13 Updated by u 12 months ago

  • Related to Bug #15846: devel and branches based on devel FTBFS: The following packages have unmet dependencies added

#14 Updated by segfault 12 months ago

intrigeri wrote:

testing (and feature/14594-asp-gui) also FTBFS because linux-image-4.17.0-2-amd64 was removed from sid

Wrt. testing, this is surprising given that branch uses a frozen set of APT snapshots that has that package. So far no build has failed on Jenkins but I've just started one to confirm my hunch.

Confirmed, the testing branch builds just fine.

OK, sorry about that.

#15 Updated by segfault 12 months ago

FTR: I reviewed this as part of #14598, and everything I didn't comment on above looks good to me.

#16 Updated by segfault 12 months ago

FTR, again (I wrote this on #14598, but it actually belongs here):

I reviewed branch feature/14594-asp-gui up to commit 93bfdb98623216573f0f997c1cfe3b9b98632a79.

What I find quite confusing is the use of the variable name package_name - it is now mostly used as the actual package name, but sometimes (e.g. tails-additional-software-config:update_packages_list), it is used as the package name + target, version etc.

#17 Updated by alant 12 months ago

From https://labs.riseup.net/code/issues/14598#note-53:

Oh, I forgot something: I think the merge of bugfix/15879-asp-2-notifications went wrong, the argument to install_additional_packages should be named upgrade_mode but it's still called ignore_old_apt_lists.

#18 Updated by segfault 12 months ago

From https://labs.riseup.net/code/issues/14598#note-48:

segfault wrote:

alant wrote:

tails-additional-software-config:
  • There are a lot of untranslatable strings in update_packages_list()

Should be fixed, thanks !

configuration-window.ui:
  • Untranslatable strings: header bar title,

The fact that the string is not extracted is a bug in xgettext, which I can't easily solve. But as there is the same string in the .desktop file, it goes in the catalog and should be used by GTK.

Did you try adding translatable="yes" to the title?

#19 Updated by intrigeri 12 months ago

  • Related to deleted (Feature #14598: Code review for Additional Software packages GUI)

#20 Updated by intrigeri 12 months ago

  • Blocks Feature #14598: Code review for Additional Software packages GUI added

#21 Updated by intrigeri 12 months ago

  • Related to deleted (Bug #15846: devel and branches based on devel FTBFS: The following packages have unmet dependencies)

#22 Updated by alant 10 months ago

  • Description updated (diff)

#23 Updated by u 10 months ago

  • Related to Bug #16060: Improve ASP code: configuration window added

#24 Updated by u 10 months ago

  • Related to Bug #16061: Improve ASP code: get rid of search_new_persistence argument added

#25 Updated by u 10 months ago

  • Related to Bug #16062: Improve ASP code: config.py added

#26 Updated by u 10 months ago

  • Status changed from In Progress to Confirmed
  • QA Check deleted (Dev Needed)

@alant: you can close this ticket if all remaining tasks have been correctly reported elsewhere.

#27 Updated by alant 10 months ago

  • Related to Bug #16034: ASP: Fix race condition when writing to packages file added

#28 Updated by alant 10 months ago

These issues have been moved:

  • configuration_window.ui: Maybe remove the install_label text, because it's overwritten in update_packages_list() anyway: #16060
  • config.py: there is a race condition between multiple calls of add_additional_packages and/or remove_additional_packages: if the config file is written between when the packages are read and when the packages are written, this change will be overwritten by the subsequent write. You could fix this by acquiring a file lock before reading the file, for example with fcntl.flock(f, fcntl.LOCK_EX), and only releasing the lock once the file was written (example: https://gitlab.com/segfault3/onionkit/blob/master/onionkit/util.py#L21). This way you could also get rid of the python3-atomicwrites package: #16034
  • config.py: both add_additional_packages() and remove_additional_packages() are only ever called with search_new_persistence=True, so why is this argument even needed? as a result of the above, get_additional_packages() and _write_config are also only called with search_new_persistence=True: #16061
  • persistence.py: get_persistence_path(): only ever called with search_new_persistence=True, return_nonexistent=False, has_unlocked_persistence(): only called with search_new_persistence=True: #16061

#29 Updated by alant 10 months ago

  • Assignee changed from alant to segfault
  • QA Check set to Ready for QA

This ticket is supposed to solve:

  • I'd suggest you set `PERSISTENCE_SETUP_USERNAME = "tails-persistence-setup"` in tailslib/__init__.py and use it in the two occurrances where this string is currently used.

tails-additional-software:

  • Why did you change the shebang from /usr/bin/env python3 to /usr/bin/python3? Usually using /usr/bin/env is preferred
  • Use logging.warning instead of deprecated logging.warn
  • _notify_failure(): * incomplete sentence in docstring: "The user has the option to edit the configuration of to view the system log" * second "details =" line uses .format() but the string doesn't contain any replacement fields
  • show_system_log(): the log file is not readable by amnesia, so gedit is not able to show it
  • apt_hook_pre(): you shouldn't use `assert` to check for conditions that could happen, because it is only evaluated if debug is True (see https://docs.python.org/3/reference/simple_stmts.html)
  • main(): What do we need the syslog handler for? Wouldn't it be better to just log to stderr (i.e. use logging.StreamHandler()) instead of logging to /dev/log? This way, if the program is run in a terminal, we get the output there directly, as would be expected, and else the output to stderr should go to the syslog anyway.

config.py:

  • it would be good to do the resetting of uid/gid in a `finally:` block, or even better, using a context manager

org.boum.tails.additional-software.policy:

  • <message> is not translatable

#30 Updated by segfault 10 months ago

  • QA Check changed from Ready for QA to Pass

alant wrote:

This ticket is supposed to solve:

  • I'd suggest you set `PERSISTENCE_SETUP_USERNAME = "tails-persistence-setup"` in tailslib/__init__.py and use it in the two occurrances where this string is currently used.

tails-additional-software:

  • Why did you change the shebang from /usr/bin/env python3 to /usr/bin/python3? Usually using /usr/bin/env is preferred
  • Use logging.warning instead of deprecated logging.warn
  • _notify_failure(): * incomplete sentence in docstring: "The user has the option to edit the configuration of to view the system log" * second "details =" line uses .format() but the string doesn't contain any replacement fields
  • show_system_log(): the log file is not readable by amnesia, so gedit is not able to show it
  • apt_hook_pre(): you shouldn't use `assert` to check for conditions that could happen, because it is only evaluated if debug is True (see https://docs.python.org/3/reference/simple_stmts.html)
  • main(): What do we need the syslog handler for? Wouldn't it be better to just log to stderr (i.e. use logging.StreamHandler()) instead of logging to /dev/log? This way, if the program is run in a terminal, we get the output there directly, as would be expected, and else the output to stderr should go to the syslog anyway.

config.py:

  • it would be good to do the resetting of uid/gid in a `finally:` block, or even better, using a context manager

LGTM

org.boum.tails.additional-software.policy:

  • <message> is not translatable

The fix for this was already merged in stable

I'm still reviewing #15983, which was done on the same branch, so I will not pass this to intrigeri's plate until I'm done with that review.

#31 Updated by segfault 10 months ago

  • Assignee changed from segfault to intrigeri

I'm still reviewing #15983, which was done on the same branch, so I will not pass this to intrigeri's plate until I'm done with that review.

That's done

#32 Updated by intrigeri 10 months ago

  • Feature Branch changed from feature/14594-asp-gui to bugfix/15838-asp-fix-non-blocking-issues

feature/14594-asp-gui does not exist anymore so given segfault's comment, I guess the branch is bugfix/15838-asp-fix-non-blocking-issues. Alan, in the future please keep such metadata up-to-date :)

#33 Updated by intrigeri 10 months ago

  • Status changed from Confirmed to In Progress
  • Assignee changed from intrigeri to alant
  • Target version changed from Tails_3.10.1 to Tails_3.11
  • QA Check deleted (Pass)

Same as #15983#note-7: merged locally, will push soon. Reassigning to Alan for the next steps (according to the ticket description, there's still issues left to be fixed).

#34 Updated by alant 10 months ago

  • Assignee changed from alant to intrigeri
  • Target version changed from Tails_3.11 to Tails_3.10.1
  • QA Check set to Pass

intrigeri wrote:

feature/14594-asp-gui does not exist anymore so given segfault's comment, I guess the branch is bugfix/15838-asp-fix-non-blocking-issues. Alan, in the future please keep such metadata up-to-date :)

Right, sorry.

Same as #15983#note-7: merged locally, will push soon. Reassigning to Alan for the next steps (according to the ticket description, there's still issues left to be fixed).

No I have moved them because it started to be very messy. See https://labs.riseup.net/code/issues/15838#note-28.

#35 Updated by intrigeri 10 months ago

  • Assignee changed from intrigeri to alant
  • QA Check changed from Pass to Info Needed

OK. So is there anything I am supposed to do? (You've reassigned to me.) If not, please set status = Fix committed and boom :)

#36 Updated by alant 10 months ago

  • Status changed from In Progress to Fix committed
  • Assignee deleted (alant)
  • QA Check changed from Info Needed to Pass

#37 Updated by CyrilBrulebois 10 months ago

  • Status changed from Fix committed to Resolved

#38 Updated by bertagaz 10 months ago

  • Related to Bug #16110: Button to remove package in ASP GUI has a wrong label added

Also available in: Atom PDF