GC incorrectly removes the temporary root file of the calling process

  • Open
  • quality assurance status badge
Details
4 participants
  • Ludovic Courtès
  • Maxim Cournoyer
  • Maxime Devos
  • Ryan Sundberg
Owner
unassigned
Submitted by
Ludovic Courtès
Severity
important
L
L
Ludovic Courtès wrote on 24 Nov 2016 15:07
(name . Eelco Dolstra)(address . eelco.dolstra@logicblox.com)
87d1hl55ur.fsf@gnu.org
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:

Toggle snippet (39 lines)
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

Notice the “removing stale temporary roots file” message.

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

Thanks,
Ludo’.
Toggle diff (27 lines)
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");
L
L
Ludovic Courtès wrote on 23 Jan 2017 23:14
control message for bug #25018
(address . control@debbugs.gnu.org)
87wpdll8yq.fsf@gnu.org
severity 25018 important
M
M
Maxim Cournoyer wrote on 7 Oct 2022 22:59
Re: bug#25018: GC incorrectly removes the temporary root file of the calling process
(name . Ludovic Courtès)(address . ludo@gnu.org)
87pmf34afr.fsf@gmail.com
Hi Ludo,

ludo@gnu.org (Ludovic Courtès) writes:

Toggle quote (92 lines)
> 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:
>
> 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
>
> Notice the “removing stale temporary roots file” message.
>
> Eelco: shouldn’t it be changed along the lines of the attached path?
>
>
> Thanks,
> Ludo’.
>
> 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");
>

I'm not Eelco, but your change LGTM. Note that the upstream version
still uses the original code [0].

I've installed the change, tested that it had the expected result:

Toggle snippet (13 lines)
reading temporary root file `/var/guix/temproots/8386'
waiting for read lock on `/var/guix/temproots/8386'
got temporary root `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo'
considering whether to delete `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo'
| cannot delete `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo' because it's a root
| cannot delete `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo' because it's still reachable
ice-9/boot-9.scm:1685:16: In procedure raise-exception:
ERROR:
1. &store-protocol-error:
message: "cannot delete path `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo' since it is still alive"
status: 1

and pushed!

Closing.


--
Thanks,
Maxim
Closed
L
L
Ludovic Courtès wrote on 10 Oct 2022 10:01
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87zge43y6a.fsf@gnu.org
Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (3 lines)
> I'm not Eelco, but your change LGTM. Note that the upstream version
> still uses the original code [0].

Right.

Toggle quote (16 lines)
> I've installed the change, tested that it had the expected result:
>
> reading temporary root file `/var/guix/temproots/8386'
> waiting for read lock on `/var/guix/temproots/8386'
> got temporary root `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo'
> considering whether to delete `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo'
> | cannot delete `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo' because it's a root
> | cannot delete `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo' because it's still reachable
> ice-9/boot-9.scm:1685:16: In procedure raise-exception:
> ERROR:
> 1. &store-protocol-error:
> message: "cannot delete path `/gnu/store/0siy93lggjw7sfdg8gsvrzafaa974h2d-foo' since it is still alive"
> status: 1
>
> and pushed!

Thank you! (Your bug triage work is much appreciated!) We could turn
the example here in a unit test; the only downside is that running the
GC in a test is expensive.

Ludo’.
Closed
M
M
Maxime Devos wrote on 10 Oct 2022 12:29
82e0b6b6-402b-8505-7fa1-24dd013c4646@telenet.be
On 10-10-2022 10:01, Ludovic Courtès wrote:
Toggle quote (8 lines)
> Hi Maxim,
>
> [...]
>> and pushed!
>
> Thank you! (Your bug triage work is much appreciated!) We could turn
> the example here in a unit test; the only downside is that running the
> GC in a test is expensive.
It should be possible to run the GC on the test store instead of
/gnu/store, no? If that's still too expensive, how about creating an
additional temporary test store only for the GC test?
Greetings,
Maxime.
Attachment: OpenPGP_0x49E3EE22191725EE.asc
Attachment: OpenPGP_signature
Closed
L
L
Ludovic Courtès wrote on 10 Oct 2022 16:53
(name . Maxime Devos)(address . maximedevos@telenet.be)
87ilkr20ib.fsf@gnu.org
Maxime Devos <maximedevos@telenet.be> skribis:

Toggle quote (12 lines)
> On 10-10-2022 10:01, Ludovic Courtès wrote:
>> Hi Maxim,
>> [...]
>>> and pushed!
>> Thank you! (Your bug triage work is much appreciated!) We could
>> turn
>> the example here in a unit test; the only downside is that running the
>> GC in a test is expensive.
>
> It should be possible to run the GC on the test store instead of
> /gnu/store, no?

Yes, that’s what I meant and several tests already do this, but it’s
quite expensive.

Toggle quote (3 lines)
> If that's still too expensive, how about creating an additional
> temporary test store only for the GC test?

Bah, that sounds complicated to me.

Ludo’.
Closed
R
R
Ryan Sundberg wrote on 10 Oct 2022 19:24
Broken test suite
(address . 25018@debbugs.gnu.org)
1266b38d-9bf5-49be-f25f-2271b63c4bf9@arctype.co
Hello, this patch seems to have broken the test suite in
tests/store.scm. My test log file is attached.

./test-env make check TESTS=tests/store.scm

Cuirass did not detect the changes since they are at such a low level:



--
Sincerely,
Ryan Sundberg
Attachment: test-suite.log
Attachment: OpenPGP_signature
L
L
Ludovic Courtès wrote on 14 Oct 2022 22:30
Re: bug#25018: GC incorrectly removes the temporary root file of the calling process
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 25018@debbugs.gnu.org)
87wn92jggr.fsf@gnu.org
Hi Maxim,

(Stripping Cc:.)

Ludovic Courtès <ludo@gnu.org> skribis:

Toggle quote (4 lines)
> Thank you! (Your bug triage work is much appreciated!) We could turn
> the example here in a unit test; the only downside is that running the
> GC in a test is expensive.

Actually, there are tests that most likely relied on the previous
behavior and are now failing in
tests/{derivations,nar,publish,pypi,store}.scm. We’ll have to look at
each one to make sure they are indeed making the wrong assumption and to
fix them.

What about reverting the change first so we can do that without
pressure and come up with a self-contained patch?

Ludo’.
M
M
Maxim Cournoyer wrote on 17 Oct 2022 03:25
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 25018@debbugs.gnu.org)
87edv7z1ev.fsf@gmail.com
Hi Ludovic!

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (16 lines)
> Hi Maxim,
>
> (Stripping Cc:.)
>
> Ludovic Courtès <ludo@gnu.org> skribis:
>
>> Thank you! (Your bug triage work is much appreciated!) We could turn
>> the example here in a unit test; the only downside is that running the
>> GC in a test is expensive.
>
> Actually, there are tests that most likely relied on the previous
> behavior and are now failing in
> tests/{derivations,nar,publish,pypi,store}.scm. We’ll have to look at
> each one to make sure they are indeed making the wrong assumption and to
> fix them.

Hmm, I hadn't seen that coming.

Toggle quote (3 lines)
> What about reverting the change first so we can do that without
> pressure and come up with a self-contained patch?

Sounds reasonable, if we can't think of an immediate fix.

--
Thanks,
Maxim
L
L
Ludovic Courtès wrote on 17 Oct 2022 10:51
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 25018@debbugs.gnu.org)
87v8oig7eg.fsf@gnu.org
Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (25 lines)
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Hi Maxim,
>>
>> (Stripping Cc:.)
>>
>> Ludovic Courtès <ludo@gnu.org> skribis:
>>
>>> Thank you! (Your bug triage work is much appreciated!) We could turn
>>> the example here in a unit test; the only downside is that running the
>>> GC in a test is expensive.
>>
>> Actually, there are tests that most likely relied on the previous
>> behavior and are now failing in
>> tests/{derivations,nar,publish,pypi,store}.scm. We’ll have to look at
>> each one to make sure they are indeed making the wrong assumption and to
>> fix them.
>
> Hmm, I hadn't seen that coming.
>
>> What about reverting the change first so we can do that without
>> pressure and come up with a self-contained patch?
>
> Sounds reasonable, if we can't think of an immediate fix.

I reverted it in eec920ba93ecb086366576e31b785962fbaf81c2.

The way forward will be to review those tests one by one, make sure they
were making the “wrong” assumption, adjust them accordingly, and
possibly add new tests. It’s not necessarily difficult but takes a bit
of time. (In the coming weeks I’m going to try and focus on more urgent
matters but I’m happy to review if you or someone else gets to it!)

Thanks,
Ludo’.
M
M
Maxim Cournoyer wrote on 18 Oct 2022 17:33
(name . Ludovic Courtès)(address . ludo@gnu.org)
877d0xw3hv.fsf@gmail.com
reopen 25018
quit

Hi,

Ludovic Courtès <ludo@gnu.org> writes:

[...]

Toggle quote (8 lines)
> I reverted it in eec920ba93ecb086366576e31b785962fbaf81c2.
>
> The way forward will be to review those tests one by one, make sure they
> were making the “wrong” assumption, adjust them accordingly, and
> possibly add new tests. It’s not necessarily difficult but takes a bit
> of time. (In the coming weeks I’m going to try and focus on more urgent
> matters but I’m happy to review if you or someone else gets to it!)

Thanks, and sorry for not noticing the breakage. I'm reopening the bug
so that we don't forget about it.

--
Thanks,
Maxim
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 25018@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 25018
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch