ASP: Fix race condition when writing to packages file
- 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_pathis 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
flockcan be used for.
- 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_remove_packages were not logged, so I fixed that too.
- Target version deleted (
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 #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.