Project

General

Profile

Bug #16034

ASP: Fix race condition when writing to packages file

Added by segfault 8 months ago. Updated 1 day ago.

Status:
In Progress
Priority:
Normal
Assignee:
Category:
-
Target version:
Start date:
10/08/2018
Due date:
% Done:

50%

Spent time:
QA Check:
Ready for QA
Feature Branch:
bugfix/16034-asp-fix-race-condition
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Additional Software Packages

Description

From #15838:
segfault wrote:

  • 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.


Related issues

Related to Tails - Bug #15838: Fix non-blocking issues identified during ASP code review Resolved 09/26/2018
Duplicates Tails - Bug #15982: race condition between multiple calls of add_additional_packages and/or remove_additional_packages Duplicate 09/26/2018

Associated revisions

Revision 7e417865 (diff)
Added by segfault 8 months ago

ASP: Fix race conditions (refs: #16034)

History

#2 Updated by segfault 8 months ago

  • Description updated (diff)

We discussed via email that I will implement a fix and alan will review it

#3 Updated by segfault 8 months ago

  • Assignee changed from segfault to alant
  • QA Check set to Ready for QA
  • Feature Branch set to bugfix/16034-asp-fix-race-condition

I implemented something, please review.

I also noticed that the exceptions raised in handle_installed_packages and handle_remove_packages were not logged, so I fixed that too.

#4 Updated by segfault 7 months ago

  • Status changed from Confirmed to In Progress

#5 Updated by alant 7 months ago

  • Duplicates Bug #15982: race condition between multiple calls of add_additional_packages and/or remove_additional_packages added

#6 Updated by alant 7 months ago

Looks OK, I'm gonna add it to a build. Did you test your code?

#7 Updated by segfault 7 months ago

alant wrote:

Looks OK, I'm gonna add it to a build. Did you test your code?

I did, but only by copying it to a 3.9.1 VM.

#8 Updated by alant 7 months ago

  • Target version changed from Tails_3.10.1 to Tails_3.11

Automated tests doesn't pass while it passes on stable. I'll investigate that later as this improvement doesn't qualify as SponsorW task.

#9 Updated by alant 7 months ago

  • Related to Bug #15838: Fix non-blocking issues identified during ASP code review added

#10 Updated by alant 7 months ago

  • % Done changed from 0 to 50
  • Affected tool set to Additional Software Packages

#11 Updated by intrigeri 6 months ago

  • Target version changed from Tails_3.11 to Tails_3.12

Too late for 3.11.

#12 Updated by alant 5 months ago

I merged stable to try again the build, but it is broken see #16226.

#13 Updated by alant 5 months ago

I think I understood the problem and pushed a fix, let's see when the build comes back!

#14 Updated by alant 5 months ago

  • Blocked by Bug #16226: Most branches FTBFS on Jenkins since the 3.11 release added

#15 Updated by alant 5 months ago

  • Blocked by deleted (Bug #16226: Most branches FTBFS on Jenkins since the 3.11 release)

#16 Updated by anonym 4 months ago

  • Target version changed from Tails_3.12 to Tails_3.13

#17 Updated by intrigeri 2 months ago

  • Target version changed from Tails_3.13 to Tails_3.14

Even if this was submitted today, we would not have the reviewer resources to merge it in time for 3.13.

#18 Updated by CyrilBrulebois 1 day ago

  • Target version changed from Tails_3.14 to Tails_3.15

Also available in: Atom PDF