From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?UTF-8?Q?Court=C3=A8s?=) Subject: bug#25018: GC incorrectly removes the temporary root file of the calling process Date: Thu, 24 Nov 2016 15:07:24 +0100 Message-ID: <87d1hl55ur.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:55468) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c9uh3-0000gI-Kh for bug-guix@gnu.org; Thu, 24 Nov 2016 09:08:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c9uh0-0005Ee-CH for bug-guix@gnu.org; Thu, 24 Nov 2016 09:08:05 -0500 Received: from debbugs.gnu.org ([208.118.235.43]:52862) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c9uh0-0005EX-8m for bug-guix@gnu.org; Thu, 24 Nov 2016 09:08:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1c9uh0-00022F-4I for bug-guix@gnu.org; Thu, 24 Nov 2016 09:08:02 -0500 Sender: "Debbugs-submit" Resent-Message-ID: Received: from eggs.gnu.org ([2001:4830:134:3::10]:55404) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c9ugh-0000fr-0d for bug-guix@gnu.org; Thu, 24 Nov 2016 09:07:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c9ugd-00051i-RU for bug-guix@gnu.org; Thu, 24 Nov 2016 09:07:42 -0500 List-Id: Bug reports for GNU Guix List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-guix-bounces+gcggb-bug-guix=m.gmane.org@gnu.org Sender: "bug-Guix" To: Eelco Dolstra Cc: h.goebel@crazy-compilers.com, 25018@debbugs.gnu.org --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hello, The =E2=80=98readTempRoots=E2=80=99 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=E2=80=99s 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=E2=80=99s 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 =3D # scheme@(guile-user)> (set-build-options s #:verbosity 10) $3 =3D #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 `c756ef12a70bad1= 0c9ac276ecd1213ea7cc3a2e6c462ba47e4f9a88756a055d0' lock released on `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo.lock' $4 =3D "/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/l= ibexec/guix/list-runtime-roots' to find additional roots [=E2=80=A6] reading temporary root file `/var/guix/temproots/4259' removing stale temporary roots file `/var/guix/temproots/4259' [=E2=80=A6] 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/1l2ml1b8ga7rwi3vlqn4wsic6z7a2c9csvi= 7mk4i1b8blw9fymn7' note: currently hard linking saves 6699.22 MiB $5 =3D ("/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo") $6 =3D 4096 --8<---------------cut here---------------end--------------->8--- Notice the =E2=80=9Cremoving stale temporary roots file=E2=80=9D message. Eelco: shouldn=E2=80=99t it be changed along the lines of the attached path? Thanks, Ludo=E2=80=99. --=-=-= Content-Type: text/x-patch Content-Disposition: inline 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 #include #include #include @@ -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"); --=-=-=--