Bug #16034

ASP: Fix race condition when writing to packages file

Added by segfault 2018-10-08 17:09:33 . Updated 2020-03-08 18:50:55 .

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

50%

Feature Branch:
bugfix/16034-asp-fix-race-condition
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Additional Software Packages
Deliverable for:

Description

From Bug #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.

Subtasks

Related issues

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

History

#1 Updated by segfault 2018-10-08 17:09:42

  • Deliverable for set to 299

#2 Updated by segfault 2018-10-08 17:11:00

  • Description updated

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

#3 Updated by segfault 2018-10-08 21:01:58

  • 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 2018-10-12 11:23:06

  • Status changed from Confirmed to In Progress

Applied in changeset commit:7e4178655f15dea499746c7d1fcea689f3456e54.

#5 Updated by alant 2018-10-15 17:23:50

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

#6 Updated by alant 2018-10-15 17:31:30

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

#7 Updated by segfault 2018-10-15 19:15:52

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 2018-10-18 17:26:10

  • Target version changed from Tails_3.10.1 to Tails_3.11
  • Deliverable for deleted (299)

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 2018-10-18 17:53:31

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

#10 Updated by alant 2018-10-18 18:13:35

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

#11 Updated by intrigeri 2018-12-10 09:03:25

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

Too late for 3.11.

#12 Updated by alant 2018-12-16 14:13:19

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

#13 Updated by alant 2018-12-16 17:08:21

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

#14 Updated by alant 2018-12-16 17:09:02

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

#15 Updated by alant 2018-12-21 15:48:05

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

#16 Updated by anonym 2019-01-30 11:59:34

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

#17 Updated by intrigeri 2019-03-17 09:16:31

  • 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 2019-05-23 21:23:28

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

#19 Updated by intrigeri 2019-06-02 14:30:20

  • QA Check deleted (Ready for QA)

(Preparing to drop the “QA Check” field as per “[Tails-dev] Proposal: Redmine workflow change”.)

#20 Updated by CyrilBrulebois 2019-07-10 10:34:09

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

#21 Updated by intrigeri 2019-08-30 17:30:55

  • Target version deleted (Tails_3.16)

Hi!

We’ve set up an automated process to ask our fellow contributors to update some tickets of theirs, in order to:

  • better reflect your plans;
  • bring down your amount of work-in-progress to a sustainable level;
  • encourage team work and increase the chances that someone finishes the work;
  • avoid a human doing ticket triaging and asking you the same questions on each such ticket.

In particular, this process identifies:

  • Stalled work-in-progress
  • Reviews waiting for a long time

However, in the current state of things, this process is not able to notice those tickets when their Target version has been repeatedly postponed by our Release Managers. Therefore, the ticket triaging team decided on Feature #16545 to remove the Target version whenever in such cases, when it does not feel realistic. This is what I’m doing on this ticket.

You now have a few options, such as:

  • Deassign yourself. That’s fine. If it really matters, someone else, possibly you, may pick it up later. Then, if this ticket is relevant for a Tails team, bring it to their attention; else, forget it and take care of yourself :)
  • If you think you can realistically come back to it and finish the work in the next 6 months, say so on this ticket, for example by setting a suitable “Target version”. This will communicate your plans to the rest of the project and ensure the task pops up on your radar at a suitable time. Of course, you can still realize later that it is not going to work as planned, and revisit today’s choice.

Cheers!

#22 Updated by alant 2020-03-08 18:50:55

  • Assignee changed from alant to segfault

I’m not sure this bug is about a real issue, it was raised during review but we werern’t able to find a fix and nobody complained since.

What do you think?