-
-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
qubes-dom0-update: prevent concurrent execution #161
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #161 +/- ##
==========================================
+ Coverage 68.89% 72.96% +4.07%
==========================================
Files 9 10 +1
Lines 1048 1154 +106
==========================================
+ Hits 722 842 +120
+ Misses 326 312 -14 ☔ View full report in Codecov by Sentry. |
Moved the condition block down after checking if the script is run with root privileges (as it requires access to /var/lock) |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024071910-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024070519-4.3&flavor=update
Failed tests9 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/105374#dependencies 5 fixed
Unstable tests
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--check-only
doesn't require root, and now it will fail. Maybe move the lock to /var/run/qubes (writable by the qubes
group)? Or create a subdir in /var/lock (using systemd-tmpfiles) that is writable by the qubes group.
This is a better idea. I even have used |
But there is still an issue - if the file was created by root, it won't be writable by the normal user. The /run/qubes dir has sgid bit set, so the group will be correct already, but the file won't be group-writable. Maybe adjust umask just for the time of opening the lock file? Or maybe open the lock for reading if already exists (to make it race-free, probably as a handling when opening for writing fails)? |
I think it would be better to create the lock only if user is root. check-only should not worry about concurrent execution. |
Check-only may be broken by parallel (started a moment later) update, or vice versa. IOW, if you allow check-only starting first (before any call by root), the issue will still happen then. |
I will implement both of the above approaches |
fixes: QubesOS/qubes-issues#6345