unofficial mirror of bug-guix@gnu.org 
 help / color / mirror / code / Atom feed
* bug#25018: GC incorrectly removes the temporary root file of the calling process
@ 2016-11-24 14:07 Ludovic Courtès
  2022-10-07 20:59 ` Maxim Cournoyer
  2022-10-10 17:24 ` bug#25018: Broken test suite Ryan Sundberg via Bug reports for GNU Guix
  0 siblings, 2 replies; 10+ messages in thread
From: Ludovic Courtès @ 2016-11-24 14:07 UTC (permalink / raw)
  To: Eelco Dolstra; +Cc: h.goebel, 25018

[-- Attachment #1: Type: text/plain, Size: 2977 bytes --]

Hello,

The ‘readTempRoots’ function in gc.cc has this:

        /* Try to acquire a write lock without blocking.  This can
           only succeed if the owning process has died.  In that case
           we don't care about its temporary roots. */
        if (lockFile(*fd, ltWrite, false)) {
            printMsg(lvlError, format("removing stale temporary roots file `%1%'") % path);
            unlink(path.c_str());

There’s a thinko here: locking the file also succeeds when the lock is
already held by the calling process.

In that case, this code ends up removing the temporary root file of
calling process, which is bad.  Here’s a sample session:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> ,use(guix)
scheme@(guile-user)> (define s (open-connection))
scheme@(guile-user)> (current-build-output-port (current-error-port))
$2 = #<output: file /dev/pts/9>
scheme@(guile-user)> (set-build-options s #:verbosity 10)
$3 = #t
scheme@(guile-user)> (add-text-to-store s "foo" "bar!")
acquiring global GC lock `/var/guix/gc.lock'
acquiring read lock on `/var/guix/temproots/4259'
acquiring write lock on `/var/guix/temproots/4259'
downgrading to read lock on `/var/guix/temproots/4259'
locking path `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo'
lock acquired on `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo.lock'
`/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo' has hash `c756ef12a70bad10c9ac276ecd1213ea7cc3a2e6c462ba47e4f9a88756a055d0'
lock released on `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo.lock'
$4 = "/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo"
scheme@(guile-user)> (delete-paths s (list $4))
acquiring global GC lock `/var/guix/gc.lock'
finding garbage collector roots...
executing `/gnu/store/l99rkv2713nl53kr3gn4akinvifsx19h-guix-0.11.0-3.7ca3/libexec/guix/list-runtime-roots' to find additional roots
[…]
reading temporary root file `/var/guix/temproots/4259'
removing stale temporary roots file `/var/guix/temproots/4259'
[…]
considering whether to delete `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo'
|   invalidating path `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo'
|   deleting `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo'
|   recursively deleting path `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo'
|   |   /gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo
deleting `/gnu/store/trash'
recursively deleting path `/gnu/store/trash'
|   /gnu/store/trash
deleting unused links...
deleting unused link `/gnu/store/.links/1l2ml1b8ga7rwi3vlqn4wsic6z7a2c9csvi7mk4i1b8blw9fymn7'
note: currently hard linking saves 6699.22 MiB
$5 = ("/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo")
$6 = 4096
--8<---------------cut here---------------end--------------->8---

Notice the “removing stale temporary roots file” message.

Eelco: shouldn’t it be changed along the lines of the attached path?

Thanks,
Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1179 bytes --]

diff --git a/nix/libstore/gc.cc b/nix/libstore/gc.cc
index 72eff52..d92388f 100644
--- a/nix/libstore/gc.cc
+++ b/nix/libstore/gc.cc
@@ -2,6 +2,7 @@
 #include "misc.hh"
 #include "local-store.hh"
 
+#include <string>
 #include <functional>
 #include <queue>
 #include <algorithm>
@@ -225,10 +226,10 @@ static void readTempRoots(PathSet & tempRoots, FDs & fds)
         //FDPtr fd(new AutoCloseFD(openLockFile(path, false)));
         //if (*fd == -1) continue;
 
-        /* Try to acquire a write lock without blocking.  This can
-           only succeed if the owning process has died.  In that case
-           we don't care about its temporary roots. */
-        if (lockFile(*fd, ltWrite, false)) {
+        /* Try to acquire a write lock without blocking.  This can only
+           succeed if the owning process has died, in which case we don't care
+           about its temporary roots, or if we are the owning process.  */
+        if (i.name != std::to_string(getpid()) && lockFile(*fd, ltWrite, false)) {
             printMsg(lvlError, format("removing stale temporary roots file `%1%'") % path);
             unlink(path.c_str());
             writeFull(*fd, "d");

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-10-18 15:34 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-24 14:07 bug#25018: GC incorrectly removes the temporary root file of the calling process Ludovic Courtès
2022-10-07 20:59 ` Maxim Cournoyer
2022-10-10  8:01   ` Ludovic Courtès
2022-10-10 10:29     ` Maxime Devos
2022-10-10 14:53       ` Ludovic Courtès
2022-10-14 20:30     ` Ludovic Courtès
2022-10-17  1:25       ` Maxim Cournoyer
2022-10-17  8:51         ` Ludovic Courtès
2022-10-18 15:33           ` Maxim Cournoyer
2022-10-10 17:24 ` bug#25018: Broken test suite Ryan Sundberg via Bug reports for GNU Guix

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).