[PATCH] guix: gexp: canonicalize file paths for import

  • Open
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Ryan Sundberg
Owner
unassigned
Submitted by
Ryan Sundberg
Severity
normal

Debbugs page

Ryan Sundberg wrote 3 weeks ago
(address . guix-patches@gnu.org)(name . Ryan Sundberg)(address . ryan@arctype.co)
0dafbea136e328cd214e7e1fb05ab91ab04b17da.1739829485.git.ryan@arctype.co
When we intern a file from the store during `imported-modules`, if the
file is a symlink (e.g., from a Guix profile), a dangling symlink can be
created in the module-import builder.

Follow any symlinks before interning the files to the store, so that the
file itself is imported and not the dangling link.


* guix/gexp.scm (imported-files/derivation): canonicalize-path

Change-Id: Ic0af90cda7c5c5819526e455cf62300e18408dbd
---
guix/gexp.scm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Toggle diff (17 lines)
diff --git a/guix/gexp.scm b/guix/gexp.scm
index e44aea6420..85351b0322 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -1576,7 +1576,7 @@ (define* (imported-files/derivation files
(define file-pair
(match-lambda
((final-path . (? string? file-name))
- (mlet %store-monad ((file (interned-file file-name
+ (mlet %store-monad ((file (interned-file (canonicalize-path file-name)
(basename final-path))))
(return (list final-path file))))
((final-path . file-like)

base-commit: 91b18baa4274a025d28f06133682a9269217730d
--
2.41.0
Ryan Sundberg wrote 3 weeks ago
Additional context for #76376 - gexp / canonicalize-path
(address . 76376@debbugs.gnu.org)
8155d903-bfd1-447d-b639-c98018a19119@arctype.co
Hello team,

This is a patch for a "deep" bug in Guix gexp processing which evokes in
circumstances when using g-expressions to build things that try to
create module closures with code that is referenced in the current
environment via symlink.

It can manifest in difficult to comprehend errors, such as "no code for
module: (guix utils)` when guix/utils.scm is correctly defined in the
load path of the program and exists (but it is a symlink, such as by
using `guix shell` to load another guix environment, e.g. where the
shell imports a different guix itself).

In my use case, I was using `guix` to build raw os disk images with my
own set of customized packages and services when this bug blocked me at
a dead stop.

The root cause of this after much complex debugging, tracing, and
reading helped me to identify the bug report from Ludo at
https://issues.guix.gnu.org/73275and understand the dangling symlink
issue. What happens here, and what this patch fixes, is that the
`interned-file` procedure will not follow symlinks, and will intern a
symlink if it is told to. In most scenarios this is harmless as the
symlinks intersect to something (e.g. guix/utils.scm) which is already
in the profile anyways, so the bug is dormant.
However, in other cases, it is possible to create a dangling symlink
here when `imported-modules` references a file which is a symlink on the
Guile %load-path, and `interned-file` in this line of gexp.scm can
intern a dangling symlink.

This patch closes that possibility by canonicalizing the path of the
interned file before loading it into the module closure path, so that
`imported-modules` will never import a dangling symlink to a guile file
used by a module-closure.

--Ryan
Ludovic Courtès wrote 2 weeks ago
Re: [bug#76376] [PATCH] guix: gexp: canonicalize file paths for import
(name . Ryan Sundberg)(address . ryan@arctype.co)
87v7t3a7m7.fsf@gnu.org
Hi,

Ryan Sundberg <ryan@arctype.co> skribis:

Toggle quote (13 lines)
> When we intern a file from the store during `imported-modules`, if the
> file is a symlink (e.g., from a Guix profile), a dangling symlink can be
> created in the module-import builder.
>
> Follow any symlinks before interning the files to the store, so that the
> file itself is imported and not the dangling link.
>
> See also: https://issues.guix.gnu.org/73275
>
> * guix/gexp.scm (imported-files/derivation): canonicalize-path
>
> Change-Id: Ic0af90cda7c5c5819526e455cf62300e18408dbd

[...]

Toggle quote (5 lines)
> ((final-path . (? string? file-name))
> - (mlet %store-monad ((file (interned-file file-name
> + (mlet %store-monad ((file (interned-file (canonicalize-path file-name)
> (basename final-path))))

Instead of calling ‘canonicalize-path’, which leads to many syscalls,
I’d suggest:

(interned-file file-name (basename final-path)
#:recursive? #f)

I believe that would have the desired effect.

Could you also add a test that reproduces the problem?

Thanks,
Ludo’.
Ludovic Courtès wrote 2 weeks ago
(name . Ryan Sundberg)(address . ryan@arctype.co)
87seo2njtj.fsf@gnu.org
Hello Ryan,

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

Toggle quote (11 lines)
>> ((final-path . (? string? file-name))
>> - (mlet %store-monad ((file (interned-file file-name
>> + (mlet %store-monad ((file (interned-file (canonicalize-path file-name)
>> (basename final-path))))
>
> Instead of calling ‘canonicalize-path’, which leads to many syscalls,
> I’d suggest:
>
> (interned-file file-name (basename final-path)
> #:recursive? #f)

It was missing one bit; attached is an version of it that works.

I chose ‘readlink*’ because it’s less expensive that
‘canonicalize-path’: only one extra syscall (readlink) when ‘file-name’
is already a regular file.

For the record, I stumbled upon this bug just today while working on
https://issues.guix.gnu.org/75810: the "imported-files does not create
symlinks" in ‘tests/gexp.scm’ would fail when running in an isolated
environment because file “x” would be a symlink to a file outside the
store.

Ludo’.
Toggle diff (16 lines)
diff --git a/guix/gexp.scm b/guix/gexp.scm
index ad51bc55b78..ddd2e1a0812 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -1584,8 +1584,9 @@ (define* (imported-files/derivation files
(define file-pair
(match-lambda
((final-path . (? string? file-name))
- (mlet %store-monad ((file (interned-file file-name
- (basename final-path))))
+ (mlet %store-monad ((file (interned-file (readlink* file-name)
+ (basename final-path)
+ #:recursive? #f)))
(return (list final-path file))))
((final-path . file-like)
(mlet %store-monad ((file (lower-object file-like system)))
Ludovic Courtès wrote 1 weeks ago
control message for bug #75810
(address . control@debbugs.gnu.org)
87mse6tjpv.fsf@gnu.org
block 75810 by 76376
quit
?
Your comment

Commenting via the web interface is currently disabled.

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

To respond to this issue using the mumi CLI, first switch to it
mumi current 76376
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
You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags
mumi command -t +confirmed -t +easy
Or, remove the moreinfo tag and set the help tag
mumi command -t -moreinfo -t +help