GNU bug report logs - #77201
[PATCH] guix: substitute-key-authorization: Fix case when acl symlink is broken

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 77201 in the body.
You can then email your comments to 77201 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rutherther <rutherther <at> ditigal.xyz>
To: guix-patches <at> gnu.org
Cc: Rutherther <rutherther <at> ditigal.xyz>
Subject: [PATCH] guix: substitute-key-authorization: Fix case when acl symlink
 is broken
Date: Sun, 23 Mar 2025 10:48:28 +0100

One possible solution for an issue when /etc/guix/acl file exists, but points
to a non-existent location. This can for example happen if one is
reinitializing the system, and remove only /gnu/store and /var/guix, keep the
rest okay. This is a major advantage of guix as compared to other distros that
usually need you to reinitialize the whole root partition. But this will leave
the user with acl file pointing to non-existent location. The file-exists?
procedure will return #f for broken symbolic links.

I think that another reason one would get this issue is, if one was booted in
a live iso, chrooted, fixing their system. They would switch generations to
one with different acl file, delete other generations gc rooting the original
acl file and then gc. One could do this approach for example when recovering
from file corruptions in the store, to get rid of the unsubstitutable paths
that can't be repaired with guix gc --verify.

I don't know if there is a better way as I am not that proficient in guile,
but I definitely think this should be fixed and it should be checked if
anything exists in that place, not that the link points to a known location.
Does Guile have a procedure for that that I am missing? If not, shouldn't
we create one in Guix? I can imagine this being a common mistake, where we
want to check if something exists at place 'x', without caring if it's
actually an accessible file. I was looking online and someone made themselves
a function 'file-exists??' that checked basically this what I did here - that
it's either a valid file on the disk, or a broken symlink.

During debugging this issue I also noticed similar issue can occur in special
files and /run/current-system with the .new files that are created with
symlink procedure without checking for their existence. While it's not likely
(especially because the symlink is moved the second it's created)
the user would have /run/current-system.new nor /bin/sh.new etc., I still
think it would be worth fixing to make sure the system can boot even in cases
where something goes horribly wrong.

* gnu/services/base.scm (substitute-key-authorization): Check if acl file is a
(broken) symbolic link

Change-Id: I2f8170606b2f4afeea48f04acfd738b04cafc7cf
---
 gnu/services/base.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index 0d2bb31190..e419d043ae 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -1845,7 +1845,7 @@ (define (substitute-key-authorization keys guix)
         ;; If the ACL already exists, move it out of the way.  Create a backup
         ;; if it's a regular file: it's likely that the user manually updated
         ;; it with 'guix archive --authorize'.
-        (if (file-exists? acl-file)
+        (if (or (file-exists? acl-file) (symbolic-link? acl-file))
             (if (and (symbolic-link? acl-file)
                      (store-file-name? (readlink acl-file)))
                 (delete-file acl-file)

base-commit: fbfd2b93831978aadbb96f32cafdab997b04c6c6
prerequisite-patch-id: cf473eb15513404ca1d287f5b7eca109c848203c
prerequisite-patch-id: a46e75bdd193acb5e276e0aa31c77197a3254699
prerequisite-patch-id: a2b4aa0a33d89ee3f6c483aeb71a783cb0e63aa9
-- 
2.49.0

Message #8 received at 77201 <at> debbugs.gnu.org (full text, mbox):

From: Ian Eure <ian <at> retrospec.tv>
To: Rutherther <rutherther <at> ditigal.xyz>
Cc: 77201 <at> debbugs.gnu.org
Subject: Re: [bug#77201] [PATCH] guix: substitute-key-authorization: Fix
 case when acl symlink is broken
Date: Sat, 29 Mar 2025 10:09:24 -0700

Hi Rutherther,

Rutherther <rutherther <at> ditigal.xyz> writes:

> One possible solution for an issue when /etc/guix/acl file 
> exists, but points
> to a non-existent location. This can for example happen if one 
> is
> reinitializing the system, and remove only /gnu/store and 
> /var/guix, keep the
> rest okay. This is a major advantage of guix as compared to 
> other distros that
> usually need you to reinitialize the whole root partition. But 
> this will leave
> the user with acl file pointing to non-existent location. The 
> file-exists?
> procedure will return #f for broken symbolic links.
>
> I think that another reason one would get this issue is, if one 
> was booted in
> a live iso, chrooted, fixing their system. They would switch 
> generations to
> one with different acl file, delete other generations gc rooting 
> the original
> acl file and then gc. One could do this approach for example 
> when recovering
> from file corruptions in the store, to get rid of the 
> unsubstitutable paths
> that can't be repaired with guix gc --verify.
>
> I don't know if there is a better way as I am not that 
> proficient in guile,
> but I definitely think this should be fixed and it should be 
> checked if
> anything exists in that place, not that the link points to a 
> known location.
> Does Guile have a procedure for that that I am missing? If not, 
> shouldn't
> we create one in Guix? I can imagine this being a common 
> mistake, where we
> want to check if something exists at place 'x', without caring 
> if it's
> actually an accessible file. I was looking online and someone 
> made themselves
> a function 'file-exists??' that checked basically this what I 
> did here - that
> it's either a valid file on the disk, or a broken symlink.
>
> During debugging this issue I also noticed similar issue can 
> occur in special
> files and /run/current-system with the .new files that are 
> created with
> symlink procedure without checking for their existence. While 
> it's not likely
> (especially because the symlink is moved the second it's 
> created)
> the user would have /run/current-system.new nor /bin/sh.new 
> etc., I still
> think it would be worth fixing to make sure the system can boot 
> even in cases
> where something goes horribly wrong.

Thanks for the explanation.

> * gnu/services/base.scm (substitute-key-authorization): Check if 
> acl file is a
> (broken) symbolic link
>
> Change-Id: I2f8170606b2f4afeea48f04acfd738b04cafc7cf
> ---
>  gnu/services/base.scm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gnu/services/base.scm b/gnu/services/base.scm
> index 0d2bb31190..e419d043ae 100644
> --- a/gnu/services/base.scm
> +++ b/gnu/services/base.scm
> @@ -1845,7 +1845,7 @@ (define (substitute-key-authorization keys 
> guix)
>          ;; If the ACL already exists, move it out of the way. 
>          Create a backup
>          ;; if it's a regular file: it's likely that the user 
>          manually updated
>          ;; it with 'guix archive --authorize'.
> -        (if (file-exists? acl-file)
> +        (if (or (file-exists? acl-file) (symbolic-link? 
> acl-file))

Guile semantics are unhelpful here: `file-exists?' returns #f for 
a broken symlink, but `symbolic-link?' raises an exception if 
given a nonexistent path.  The means that if /etc/guix/acl doesn’t 
exist, an exception will be raised.  There doesn’t appear to be a 
simple way to determine if a file exists which doesn’t resolve 
symlinks, which I think is a Guile bug.

Thinking through the possible situations here:

If /etc/guix/acl is a good symlink pointing into /gnu/store -> 
delete it.

If /etc/guix/acl is a broken symlink pointing anywhere -> delete 
it.

If /etc/guix/acl is a file -> rename it to ".bak"

else /etc/guix/acl must be missing -> mkdir-p /etc/acl.

...then populate /etc/guix/acl.

I think the right move here is to refactor the nested `if's into a 
cond to simplify the logic, and wrap `symbolic-link?' in 
`with-exception-handler' (possibly `let’-binding its result, since 
multiple things need it).

Thanks,

 -- Ian

Message #11 received at 77201 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> chbouib.org>
To: Ian Eure <ian <at> retrospec.tv>
Cc: Rutherther <rutherther <at> ditigal.xyz>, 77201 <at> debbugs.gnu.org
Subject: Re: [bug#77201] [PATCH] guix: substitute-key-authorization: Fix
 case when acl symlink is broken
Date: Tue, 15 Apr 2025 13:30:20 +0200

Hello,

Ian Eure <ian <at> retrospec.tv> writes:

>> -        (if (file-exists? acl-file)
>> +        (if (or (file-exists? acl-file) (symbolic-link? acl-file))
>
> Guile semantics are unhelpful here: `file-exists?' returns #f for a
> broken symlink, but `symbolic-link?' raises an exception if given a
> nonexistent path.

I would go back to the fundamentals:

  (match (and=> (false-if-exception (lstat acl-file)) stat:type)
    (#f ;file does not exist
     …)
    ('symlink
     …)
    (_
     …))

HTH!

Ludo’.

Message #14 received at 77201 <at> debbugs.gnu.org (full text, mbox):

From: Rutherther <rutherther <at> ditigal.xyz>
To: Ludovic Courtès <ludo <at> chbouib.org>, Ian Eure
 <ian <at> retrospec.tv>
Cc: 77201 <at> debbugs.gnu.org
Subject: Re: [bug#77201] [PATCH] guix: substitute-key-authorization: Fix
 case when acl symlink is broken
Date: Tue, 15 Apr 2025 20:26:19 +0200

Hi Ludo,

Ludovic Courtès <ludo <at> chbouib.org> writes:

> Hello,
>
> Ian Eure <ian <at> retrospec.tv> writes:
>
>>> -        (if (file-exists? acl-file)
>>> +        (if (or (file-exists? acl-file) (symbolic-link? acl-file))
>>
>> Guile semantics are unhelpful here: `file-exists?' returns #f for a
>> broken symlink, but `symbolic-link?' raises an exception if given a
>> nonexistent path.
>
> I would go back to the fundamentals:
>
>   (match (and=> (false-if-exception (lstat acl-file)) stat:type)
>     (#f ;file does not exist
>      …)
>     ('symlink
>      …)
>     (_
>      …))

This definitely helps, thanks, I am still not that skilled in Scheme, so
I wouldn't think about false-if-exception, and using match would also be
hard for me to figure out.

I have this now (untested for now)
```
(match (and=> (false-if-exception (lstat acl-file)) stat:type)
          (#f #f) ; File doesn't exist
          ('symlink ; Delete symlink pointing to store; backup otherwise.
           (if (or (store-file-name? (readlink acl-file)) ; Store symlink
                   (not (file-exists? acl-file))) ; Broken symlink
               (delete-file acl-file)
               (rename-file acl-file (string-append acl-file ".bak"))))
          (_ ; Backup
           (rename-file acl-file (string-append acl-file ".bak"))))
```
I will probably also make this into a reusable function in guix utils
build, but I have been thinking about a good name and couldn't come up with
one for a week! Programmers problems, I guess.

WDYT

Thanks,
Rutherther

Message #17 received at 77201 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> chbouib.org>
To: Rutherther <rutherther <at> ditigal.xyz>
Cc: 77201 <at> debbugs.gnu.org, Ian Eure <ian <at> retrospec.tv>
Subject: Re: [bug#77201] [PATCH] guix: substitute-key-authorization: Fix
 case when acl symlink is broken
Date: Wed, 16 Apr 2025 16:11:35 +0200

Hi,

Rutherther <rutherther <at> ditigal.xyz> writes:

>>   (match (and=> (false-if-exception (lstat acl-file)) stat:type)
>>     (#f ;file does not exist
>>      …)
>>     ('symlink
>>      …)
>>     (_
>>      …))
>
> This definitely helps, thanks, I am still not that skilled in Scheme, so
> I wouldn't think about false-if-exception, and using match would also be
> hard for me to figure out.
>
> I have this now (untested for now)
> ```
> (match (and=> (false-if-exception (lstat acl-file)) stat:type)
>           (#f #f) ; File doesn't exist

Indentation is off (see above.)  Also (I’m nitpicking!), margin comment
should not be a full sentence, so rather:

  (#f #f)  ;file doesn't exist

>           ('symlink ; Delete symlink pointing to store; backup otherwise.
>            (if (or (store-file-name? (readlink acl-file)) ; Store symlink
>                    (not (file-exists? acl-file))) ; Broken symlink
>                (delete-file acl-file)
>                (rename-file acl-file (string-append acl-file ".bak"))))
>           (_ ; Backup
>            (rename-file acl-file (string-append acl-file ".bak"))))

Should be good!

> I will probably also make this into a reusable function in guix utils
> build,

Maybe not: (guix build utils) is rarely updated because it entails world
rebuilds, which also means that it’s interfaces must be very generic and
bullet-proof.  So I would keep it local for now.

Thanks,
Ludo’.

Message #20 received at 77201 <at> debbugs.gnu.org (full text, mbox):

From: Rutherther <rutherther <at> ditigal.xyz>
To: Ludovic Courtès <ludo <at> chbouib.org>
Cc: 77201 <at> debbugs.gnu.org, Ian Eure <ian <at> retrospec.tv>
Subject: Re: [bug#77201] [PATCH] guix: substitute-key-authorization: Fix
 case when acl symlink is broken
Date: Sun, 20 Apr 2025 12:21:09 +0200

Hello Ludo,

Ludovic Courtès <ludo <at> chbouib.org> writes:

> Hi,
>
> Rutherther <rutherther <at> ditigal.xyz> writes:
>

> Indentation is off (see above.)

Oh yeah, I didn't take much care with pasting it to an e-mail and it
became off somehow even though in code I have proper indentation.

> Also (I’m nitpicking!), margin comment
> should not be a full sentence, so rather:
>
>   (#f #f)  ;file doesn't exist

Oh, okay, I wasn't aware of these rules or even what 'margin' comments
are. But to be clear, you also agree the comments make sense here? It
seemed to me it's a bit hard to follow the reasoning here from the code itself.

>
> Should be good!

I am going testing now in a VM, then I will submit a patch later.

>
>> I will probably also make this into a reusable function in guix utils
>> build,
>
> Maybe not: (guix build utils) is rarely updated because it entails world
> rebuilds, which also means that it’s interfaces must be very generic and
> bullet-proof.  So I would keep it local for now.

Good point, I haven't thought of that.

Thank you,
Rutherther

Message #23 received at 77201 <at> debbugs.gnu.org (full text, mbox):

From: Rutherther <rutherther <at> ditigal.xyz>
To: 77201 <at> debbugs.gnu.org
Cc: Rutherther <rutherther <at> ditigal.xyz>
Subject: [PATCH v2] guix: substitute-key-authorization: Fix case when acl
 symlink is broken
Date: Mon, 28 Apr 2025 10:07:53 +0200

One possible solution for an issue when /etc/guix/acl file exists, but points
to a non-existent location. This can for example happen if one is
reinitializing the system, and remove only /gnu/store and /var/guix, keep the
rest okay. This is a major advantage of guix as compared to other distros that
usually need you to reinitialize the whole root partition. But this will leave
the user with acl file pointing to non-existent location. The file-exists?
procedure will return #f for broken symbolic links.

I think that another reason one would get this issue is, if one was booted in
a live iso, chrooted, fixing their system. They would switch generations to
one with different acl file, delete other generations gc rooting the original
acl file and then gc. One could do this approach for example when recovering
from file corruptions in the store, to get rid of the unsubstitutable paths
that can't be repaired with guix gc --verify.

This fixes the issue by looking for type of a file through lstat, instead of
relying on file-exists?. If the symlink is a broken symlink, it is
removed. Other than that the old behavior is kept:
- If regular file, back it up
- If symlink pointing to the store, remove it
- If symlink not pointing to the store, back it up

* gnu/services/base.scm (substitute-key-authorization): Check if acl file is a
(broken) symbolic link

Change-Id: I2f8170606b2f4afeea48f04acfd738b04cafc7cf
---
 gnu/services/base.scm | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index 8c6563c99d..02b4274e9d 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -1841,17 +1841,22 @@ (define (substitute-key-authorization keys guix)
 
   (with-imported-modules '((guix build utils))
     #~(begin
-        (use-modules (guix build utils))
+        (use-modules (guix build utils)
+                     (ice-9 match))
         (define acl-file #$%acl-file)
         ;; If the ACL already exists, move it out of the way.  Create a backup
         ;; if it's a regular file: it's likely that the user manually updated
         ;; it with 'guix archive --authorize'.
-        (if (file-exists? acl-file)
-            (if (and (symbolic-link? acl-file)
-                     (store-file-name? (readlink acl-file)))
-                (delete-file acl-file)
-                (rename-file acl-file (string-append acl-file ".bak")))
-            (mkdir-p (dirname acl-file)))
+        (match (and=> (false-if-exception (lstat acl-file)) stat:type)
+          (#f #f) ;file doesn't exist
+          ('symlink ;delete symlink pointing to store; backup otherwise.
+           (if (or (store-file-name? (readlink acl-file)) ;store symlink
+                   (not (file-exists? acl-file))) ;broken symlink
+               (delete-file acl-file)
+               (rename-file acl-file (string-append acl-file ".bak"))))
+          (_ ;backup
+           (rename-file acl-file (string-append acl-file ".bak"))))
+        (mkdir-p (dirname acl-file))
 
         ;; Installed the declared ACL.
         (symlink #+default-acl acl-file))))

base-commit: 56999614a45449c4b93c8614540210b609c2b356
-- 
2.49.0

Message #26 received at 77201 <at> debbugs.gnu.org (full text, mbox):

From: Rutherther <rutherther <at> ditigal.xyz>
To: Ludovic Courtès <ludo <at> chbouib.org>
Cc: 77201 <at> debbugs.gnu.org, Ian Eure <ian <at> retrospec.tv>
Subject: Re: [bug#77201] [PATCH] guix: substitute-key-authorization: Fix
 case when acl symlink is broken
Date: Mon, 28 Apr 2025 10:14:23 +0200

Apologies for the delay, I somehow didn't get to testing in a VM last
week, today I finally tested it, found a few issues, fixed them, and it
seems to be working fine for me. I tried to test all possible cases:

- No acl file
- Regular file
- Symlink to store
- Symlink out of store
- Broken symlink

- .bak (not) existing

I've kept it directly in this function upon Ludo's comment about
rebuilding the whole world if it was added to guix/build/utils, I can
create a new guix/build file if you think it makes sense to have it
reusable, or put it to another one(?).

Since this is activation logic, I think it's important to thoroughly
test it - should a new VM tests be added for this, or is it unnecesasry
and the current tests suffice? If something went wrong, the tests would
fail to boot (I haven't tried using system tests defined in guix yet, I
will try to run them).
Will QA run them for this commit or is it ran only for defined branches?

Submitted v2.

Regards,
Rutherther

Message #29 received at 77201 <at> debbugs.gnu.org (full text, mbox):

From: Rutherther <rutherther <at> ditigal.xyz>
To: Ludovic Courtès <ludo <at> chbouib.org>
Cc: 77201 <at> debbugs.gnu.org, Ian Eure <ian <at> retrospec.tv>
Subject: Re: [bug#77201] [PATCH] guix: substitute-key-authorization: Fix
 case when acl symlink is broken
Date: Fri, 02 May 2025 20:33:48 +0200

I was skimming through the code and I found out install-channels-file
has the same issue as substitute-key-authorization has with this
symlinking. I suppose I will make this into a generic function after
all, so that it can be used in both of those functions.

Just wondering if I should make it a gexp or put it to some of the
guix/build/ files (or a new one?)

"Rutherther" <rutherther <at> ditigal.xyz> writes:

> Apologies for the delay, I somehow didn't get to testing in a VM last
> week, today I finally tested it, found a few issues, fixed them, and it
> seems to be working fine for me. I tried to test all possible cases:
>
> - No acl file
> - Regular file
> - Symlink to store
> - Symlink out of store
> - Broken symlink
>
> - .bak (not) existing
>
> I've kept it directly in this function upon Ludo's comment about
> rebuilding the whole world if it was added to guix/build/utils, I can
> create a new guix/build file if you think it makes sense to have it
> reusable, or put it to another one(?).
>
> Since this is activation logic, I think it's important to thoroughly
> test it - should a new VM tests be added for this, or is it unnecesasry
> and the current tests suffice? If something went wrong, the tests would
> fail to boot (I haven't tried using system tests defined in guix yet, I
> will try to run them).
> Will QA run them for this commit or is it ran only for defined branches?
>
> Submitted v2.
>
> Regards,
> Rutherther

Message #34 received at 77201-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Rutherther <rutherther <at> ditigal.xyz>
Cc: 77201-done <at> debbugs.gnu.org, Ian Eure <ian <at> retrospec.tv>
Subject: Re: bug#77201: [PATCH] guix: substitute-key-authorization: Fix case
 when acl symlink is broken
Date: Mon, 05 May 2025 12:19:21 +0200

Hi,

Rutherther <rutherther <at> ditigal.xyz> writes:

> One possible solution for an issue when /etc/guix/acl file exists, but points
> to a non-existent location. This can for example happen if one is
> reinitializing the system, and remove only /gnu/store and /var/guix, keep the
> rest okay. This is a major advantage of guix as compared to other distros that
> usually need you to reinitialize the whole root partition. But this will leave
> the user with acl file pointing to non-existent location. The file-exists?
> procedure will return #f for broken symbolic links.
>
> I think that another reason one would get this issue is, if one was booted in
> a live iso, chrooted, fixing their system. They would switch generations to
> one with different acl file, delete other generations gc rooting the original
> acl file and then gc. One could do this approach for example when recovering
> from file corruptions in the store, to get rid of the unsubstitutable paths
> that can't be repaired with guix gc --verify.
>
> This fixes the issue by looking for type of a file through lstat, instead of
> relying on file-exists?. If the symlink is a broken symlink, it is
> removed. Other than that the old behavior is kept:
> - If regular file, back it up
> - If symlink pointing to the store, remove it
> - If symlink not pointing to the store, back it up
>
> * gnu/services/base.scm (substitute-key-authorization): Check if acl file is a
> (broken) symbolic link
>
> Change-Id: I2f8170606b2f4afeea48f04acfd738b04cafc7cf

Applied after changing the terminology from “broken symlink” to
“dangling symlink”, which I think is more widely used (and easier to
find while grepping!).

Thanks,
Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.