bug#19540: minor: module path picking up ./././...

bug#19540: minor: module path picking up ./././...

[Matt Wette]

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

guile 2.0.11 on macos (darwin) 10.9.5.

This seems very minor to but I thought I'd report it.  If I ",reload" a module in the current directory it seems to pick up extra "./" parts in the path.

...
;;; compiling ././././././././././././lalr1.scm
;;; compiled /Users/mwette/.cache/guile/ccache/2.0-LE-8-2.0/Users/mwette/proj/scheme/myproj/lalr1/lalr1.scm.go
scheme@(guile-user)> (system "touch lalr1.scm")
$5 = 0
scheme@(guile-user)> ,reload (lalr1)
;;; note: source file ./././././././././././././lalr1.scm
;;;       newer than compiled /Users/mwette/.cache/guile/ccache/2.0-LE-8-2.0/Users/mwette/proj/scheme/myproj/lalr1/lalr1.scm.go
;;; compiling ./././././././././././././lalr1.scm
;;; compiled /Users/mwette/.cache/guile/ccache/2.0-LE-8-2.0/Users/mwette/proj/scheme/myproj/lalr1/lalr1.scm.go
scheme@(guile-user)> (system "touch lalr1.scm")
$6 = 0
scheme@(guile-user)> ,reload (lalr1)
;;; note: source file ././././././././././././././lalr1.scm
;;;       newer than compiled /Users/mwette/.cache/guile/ccache/2.0-LE-8-2.0/Users/mwette/proj/scheme/myproj/lalr1/lalr1.scm.go
;;; compiling ././././././././././././././lalr1.scm
;;; compiled /Users/mwette/.cache/guile/ccache/2.0-LE-8-2.0/Users/mwette/proj/scheme/myproj/lalr1/lalr1.scm.go
scheme@(guile-user)> 


[-- Attachment #2: Type: text/html, Size: 2819 bytes --]

bug#19540: repeated ./././ in compiled modules

[Matt Wette]

Some more info:

1) I found that this problem persists across restarts of guile.  I have been debugging a module in current dir and I am seeing the path extend an extra "./" every time I type ",reload (lalr1).

2) My environment includes
   GUILE_LOAD_PATH=.:/Users/mwette/opt/guile

3) If I change the above to the following, the problem goes away (but the following solution not satisfactory IMO).
  GUILE_LOAD_PATH=:/Users/mwette/opt/guile   # dot replaced with blank

Take care,
Matt

bug#19540: also generates problem for debugger tracebacks

[Matt Wette]

I had another problem with the "." in my GUILE_LOAD_PATH, I believe.
If my code encountered an error, the debugger traceback would not list file/line information.

bug#19540: repeated ./././ in compiled modules

[Ludovic Courtès]

Matt Wette <mwette@alumni.caltech.edu> skribis:

> 1) I found that this problem persists across restarts of guile.  I have been debugging a module in current dir and I am seeing the path extend an extra "./" every time I type ",reload (lalr1).
>
> 2) My environment includes
>    GUILE_LOAD_PATH=.:/Users/mwette/opt/guile

The problem stems from the ‘.’ entry in the search path.  On one hand
this is perfectly valid; on the other hand, it’s usually frowned upon,
because you may end up executing possibly malicious code that just
happens to be in $PWD.

All in all, I recommend using only absolute directory names in the
search paths, which will also solve the initial problem.

Can you confirm?

Thanks,
Ludo’.

bug#19540: repeated ./././ in compiled modules

[Matt Wette]

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

Absolute paths work.  But this is really unsatisfactory IMO.  I develop code in modules and do so in many directories.  It would be quite painful to just use absolute paths.

I don't see what the big security thread is.   If really a problem, why does guile allow relative paths?

For comparison, Python will load modules in the current directory.

Nonetheless, I think "." is breaking the traceback with the "./" being added on reload.  When I was using "." I wasn't getting file, line information in tracebacks.  Now, without GUILE_LOAD_PATH set to ":$HOME/opt/guile" I get traceback info for modules in my current directory.

I think the following may be a candidate fix?

code is from guile-2.0.11, in file boot-9.scm, near line 1683:

(define (in-vicinity vicinity file)
  (let ((tail (let ((len (string-length vicinity)))
                (if (zero? len)
                    #f
                    (string-ref vicinity (- len 1))))))
    (string-append vicinity
                   (if (or (not tail) (file-name-separator? tail))
                       ""
                       file-name-separator-string)
                   file)))
;; FIX?
(define (new-in-vicinity vicinity file)
  (let ((tail (let ((len (string-length vicinity)))
                (if (or (zero? len) (string=? "." vicinity))
                    #f
                    (string-ref vicinity (- len 1))))))
    (string-append vicinity
                   (if (or (not tail) (file-name-separator? tail))
                       ""
                       file-name-separator-string)
                   file)))


On Jan 19, 2015, at 12:28 PM, Ludovic Courtès <ludo@gnu.org> wrote:

> Matt Wette <mwette@alumni.caltech.edu> skribis:
> 
>> 1) I found that this problem persists across restarts of guile.  I have been debugging a module in current dir and I am seeing the path extend an extra "./" every time I type ",reload (lalr1).
>> 
>> 2) My environment includes
>>   GUILE_LOAD_PATH=.:/Users/mwette/opt/guile
> 
> The problem stems from the ‘.’ entry in the search path.  On one hand
> this is perfectly valid; on the other hand, it’s usually frowned upon,
> because you may end up executing possibly malicious code that just
> happens to be in $PWD.
> 
> All in all, I recommend using only absolute directory names in the
> search paths, which will also solve the initial problem.
> 
> Can you confirm?
> 
> Thanks,
> Ludo’.


[-- Attachment #2: Type: text/html, Size: 5391 bytes --]

bug#19540: repeated ./././ in compiled modules

[Ludovic Courtès]

Matt Wette <mwette@alumni.caltech.edu> skribis:

> Absolute paths work.  But this is really unsatisfactory IMO.  I develop code in modules and do so in many directories.  It would be quite painful to just use absolute paths.
>
> I don't see what the big security thread is.

Suppose we’re working on the same machine and I install
/tmp/ice-9/regex.scm (say), which just does:

  (system "rm -rf /")

If I can tweak you into running Guile with /tmp as its current working
directory, you lose.

> If really a problem, why does guile allow relative paths?
>
> For comparison, Python will load modules in the current directory.
>
> Nonetheless, I think "." is breaking the traceback with the "./" being
> added on reload.  When I was using "." I wasn't getting file, line
> information in tracebacks.  Now, without GUILE_LOAD_PATH set to
> ":$HOME/opt/guile" I get traceback info for modules in my current
> directory.

Actually the problem stems from the “file name relative
canonicalization”, the process by which Guile attaches a file name
relative to the search path to an open port:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> (add-to-load-path ".")
scheme@(guile-user)> %load-path
$2 = ("." [...])
scheme@(guile-user)> (getcwd)
$3 = "/home/ludo/src/guile/module/ice-9"
scheme@(guile-user)> (open-input-file (search-path %load-path "boot-9.scm"))
$4 = #<input: ./boot-9.scm 11>
scheme@(guile-user)> (open-input-file (search-path %load-path "./boot-9.scm"))
$5 = #<input: ././boot-9.scm 12>
scheme@(guile-user)> (open-input-file (search-path %load-path "././boot-9.scm"))
$6 = #<input: ./././boot-9.scm 13>
--8<---------------cut here---------------end--------------->8---

This could be fixed either in ‘search-path’ or in
‘scm_i_relativize_path’, but the former sounds like a better place.

However, I’m unsure of the effect of changing the result of
‘search-path’ in those cases, so I’d rather leave things unchanged.

Thoughts?

Ludo’.

bug#19540: repeated ./././ in compiled modules

[Andy Wingo]

Hi!

On Tue 20 Jan 2015 22:12, ludo@gnu.org (Ludovic Courtès) writes:

> scheme@(guile-user)> (add-to-load-path ".")
> scheme@(guile-user)> %load-path
> $2 = ("." [...])
> scheme@(guile-user)> (getcwd)
> $3 = "/home/ludo/src/guile/module/ice-9"
> scheme@(guile-user)> (open-input-file (search-path %load-path "boot-9.scm"))
> $4 = #<input: ./boot-9.scm 11>
> scheme@(guile-user)> (open-input-file (search-path %load-path "./boot-9.scm"))
> $5 = #<input: ././boot-9.scm 12>
> scheme@(guile-user)> (open-input-file (search-path %load-path "././boot-9.scm"))
> $6 = #<input: ./././boot-9.scm 13>
>
> This could be fixed either in ‘search-path’ or in
> ‘scm_i_relativize_path’, but the former sounds like a better place.

I think the latter is actually better.  `relativize-path' tries to find
an element of %load-path which is a prefix of *the canonicalized version
of* a file name.  We can't expect that to work unless we also
canonicalize the elements of %load-path, for the purposes of
relativizing.  We don't want to canonicalize them eagerly or
persistently, as that would access symlinks at the wrong time; just as
needed inside relativize-path.  I pushed the attached fix to master.  If
it sounds good to you let's push it to stable-2.0 as well.  WDYT?

Andy

commit 9a951678713557b548415d32eae6d63d039bf652
Author: Andy Wingo <wingo@pobox.com>
Date:   Thu Jun 23 10:03:10 2016 +0200

    Fix relative file name canonicalization on paths with "."
    
    * libguile/filesys.c (scm_i_relativize_path): Canonicalize the file
      names elements that we will be using as prefixes.  Fixes the case
      where a load path contains a relative file name: #19540.
    * test-suite/tests/ports.test ("%file-port-name-canonicalization"): Add
      tests that elements of the load path are canonicalized.

diff --git a/libguile/filesys.c b/libguile/filesys.c
index 7674498..25501ef 100644
--- a/libguile/filesys.c
+++ b/libguile/filesys.c
@@ -1614,22 +1614,40 @@ SCM_DEFINE (scm_canonicalize_path, "canonicalize-path", 1, 0, 0,
 SCM
 scm_i_relativize_path (SCM path, SCM in_path)
 {
-  char *str, *canon;
   SCM scanon;
   
-  str = scm_to_locale_string (path);
-  canon = canonicalize_file_name (str);
-  free (str);
-  
-  if (!canon)
-    return SCM_BOOL_F;
+  {
+    char *str, *canon;
 
-  scanon = scm_take_locale_string (canon);
+    str = scm_to_locale_string (path);
+    canon = canonicalize_file_name (str);
+    free (str);
 
+    if (!canon)
+      return SCM_BOOL_F;
+
+    scanon = scm_take_locale_string (canon);
+  }
+  
   for (; scm_is_pair (in_path); in_path = scm_cdr (in_path))
     {
       SCM dir = scm_car (in_path);
-      size_t len = scm_c_string_length (dir);
+      size_t len;
+
+      /* Try to canonicalize DIR, since we have canonicalized PATH.  */
+      {
+        char *str, *canon;
+
+        str = scm_to_locale_string (dir);
+        canon = canonicalize_file_name (str);
+        free (str);
+  
+        if (canon)
+          dir = scm_from_locale_string (canon);
+        free (canon);
+      }
+
+      len = scm_c_string_length (dir);
 
       /* When DIR is empty, it means "current working directory".  We
 	 could set DIR to (getcwd) in that case, but then the
diff --git a/test-suite/tests/ports.test b/test-suite/tests/ports.test
index dfa430e..ea8eaa7 100644
--- a/test-suite/tests/ports.test
+++ b/test-suite/tests/ports.test
@@ -1865,14 +1865,15 @@
       (with-fluids ((%file-port-name-canonicalization 'relative))
         (port-filename (open-input-file "/dev/null")))))
 
+  (pass-if-equal "relative canonicalization with /dev/.." "dev/null"
+    (with-load-path (cons "/dev/.." %load-path)
+      (with-fluids ((%file-port-name-canonicalization 'relative))
+        (port-filename (open-input-file "/dev/null")))))
+
   (pass-if-equal "relative canonicalization from ice-9" "ice-9/q.scm"
-    ;; If an entry in %LOAD-PATH is not canonical, then
-    ;; `scm_i_relativize_path' is unable to do its job.
-    (if (equal? (map canonicalize-path %load-path) %load-path)
-        (with-fluids ((%file-port-name-canonicalization 'relative))
-          (port-filename
-           (open-input-file (%search-load-path "ice-9/q.scm"))))
-        (throw 'unresolved)))
+    (with-fluids ((%file-port-name-canonicalization 'relative))
+      (port-filename
+       (open-input-file (%search-load-path "ice-9/q.scm")))))
 
   (pass-if-equal "absolute canonicalization from ice-9"
       (canonicalize-path

bug#19540: repeated ./././ in compiled modules

[Ludovic Courtès]

Andy Wingo <wingo@pobox.com> skribis:

> commit 9a951678713557b548415d32eae6d63d039bf652
> Author: Andy Wingo <wingo@pobox.com>
> Date:   Thu Jun 23 10:03:10 2016 +0200
>
>     Fix relative file name canonicalization on paths with "."
>     
>     * libguile/filesys.c (scm_i_relativize_path): Canonicalize the file
>       names elements that we will be using as prefixes.  Fixes the case
>       where a load path contains a relative file name: #19540.
>     * test-suite/tests/ports.test ("%file-port-name-canonicalization"): Add
>       tests that elements of the load path are canonicalized.
>
> diff --git a/libguile/filesys.c b/libguile/filesys.c
> index 7674498..25501ef 100644
> --- a/libguile/filesys.c
> +++ b/libguile/filesys.c
> @@ -1614,22 +1614,40 @@ SCM_DEFINE (scm_canonicalize_path, "canonicalize-path", 1, 0, 0,
>  SCM
>  scm_i_relativize_path (SCM path, SCM in_path)

[...]

>    for (; scm_is_pair (in_path); in_path = scm_cdr (in_path))
>      {
>        SCM dir = scm_car (in_path);
> -      size_t len = scm_c_string_length (dir);
> +      size_t len;
> +
> +      /* Try to canonicalize DIR, since we have canonicalized PATH.  */
> +      {
> +        char *str, *canon;
> +
> +        str = scm_to_locale_string (dir);
> +        canon = canonicalize_file_name (str);
> +        free (str);
> +  
> +        if (canon)
> +          dir = scm_from_locale_string (canon);
> +        free (canon);
> +      }
> +
> +      len = scm_c_string_length (dir);
>  
>        /* When DIR is empty, it means "current working directory".  We
>  	 could set DIR to (getcwd) in that case, but then the

‘canonicalize_file_name’ is costly: roughly one syscall per file name
component.

IIUC, ‘canonicalize_file_name’ is now called once for each ‘%load-path’
entry and file name that we canonicalize.  Is this correct?

Ludo’.

bug#19540: repeated ./././ in compiled modules

[Andy Wingo]

Hi :)

On Thu 23 Jun 2016 15:06, ludo@gnu.org (Ludovic Courtès) writes:

> Andy Wingo <wingo@pobox.com> skribis:
>
>> commit 9a951678713557b548415d32eae6d63d039bf652
>> Author: Andy Wingo <wingo@pobox.com>
>> Date:   Thu Jun 23 10:03:10 2016 +0200
>>
>>     Fix relative file name canonicalization on paths with "."
>>     
>>     * libguile/filesys.c (scm_i_relativize_path): Canonicalize the file
>>       names elements that we will be using as prefixes.  Fixes the case
>>       where a load path contains a relative file name: #19540.
>>     * test-suite/tests/ports.test ("%file-port-name-canonicalization"): Add
>>       tests that elements of the load path are canonicalized.
>>
> ‘canonicalize_file_name’ is costly: roughly one syscall per file name
> component.
>
> IIUC, ‘canonicalize_file_name’ is now called once for each ‘%load-path’
> entry and file name that we canonicalize.  Is this correct?

That's correct, but only for relative canonicalization, which is in
practive only when loading Scheme files from source.  Seems out of the
hot path; what do you think?

Andy

bug#19540: repeated ./././ in compiled modules

[Ludovic Courtès]

Hello!

Andy Wingo <wingo@pobox.com> skribis:

> On Thu 23 Jun 2016 15:06, ludo@gnu.org (Ludovic Courtès) writes:
>
>> Andy Wingo <wingo@pobox.com> skribis:
>>
>>> commit 9a951678713557b548415d32eae6d63d039bf652
>>> Author: Andy Wingo <wingo@pobox.com>
>>> Date:   Thu Jun 23 10:03:10 2016 +0200
>>>
>>>     Fix relative file name canonicalization on paths with "."
>>>     
>>>     * libguile/filesys.c (scm_i_relativize_path): Canonicalize the file
>>>       names elements that we will be using as prefixes.  Fixes the case
>>>       where a load path contains a relative file name: #19540.
>>>     * test-suite/tests/ports.test ("%file-port-name-canonicalization"): Add
>>>       tests that elements of the load path are canonicalized.
>>>
>> ‘canonicalize_file_name’ is costly: roughly one syscall per file name
>> component.
>>
>> IIUC, ‘canonicalize_file_name’ is now called once for each ‘%load-path’
>> entry and file name that we canonicalize.  Is this correct?
>
> That's correct, but only for relative canonicalization, which is in
> practive only when loading Scheme files from source.  Seems out of the
> hot path; what do you think?

I think it’s likely to have a noticeable impact on startup time for
projects with a large number of modules like Guix.

For instance, commands like ‘guix package -A’ or ‘guix build foo’ load
all the modules.  The impact will be smaller on a laptop with an SSD
than on an NFS mount, where it’s likely going to be terrrible (this
could be tested using the ‘delay’ device mapper.)

WDYT?

Ludo’.

bug#19540: repeated ./././ in compiled modules

[Andy Wingo]

Hi :)

On Fri 24 Jun 2016 10:28, ludo@gnu.org (Ludovic Courtès) writes:

> Hello!
>
> Andy Wingo <wingo@pobox.com> skribis:
>
>> On Thu 23 Jun 2016 15:06, ludo@gnu.org (Ludovic Courtès) writes:
>>
>>> ‘canonicalize_file_name’ is costly: roughly one syscall per file name
>>> component.
>>>
>>> IIUC, ‘canonicalize_file_name’ is now called once for each ‘%load-path’
>>> entry and file name that we canonicalize.  Is this correct?
>>
>> That's correct, but only for relative canonicalization, which is in
>> practive only when loading Scheme files from source.  Seems out of the
>> hot path; what do you think?
>
> I think it’s likely to have a noticeable impact on startup time for
> projects with a large number of modules like Guix.

Aren't they usually compiled?  Loading compiled files will not
go through this path AFAIU.

> For instance, commands like ‘guix package -A’ or ‘guix build foo’ load
> all the modules.  The impact will be smaller on a laptop with an SSD
> than on an NFS mount, where it’s likely going to be terrrible (this
> could be tested using the ‘delay’ device mapper.)

Oh I'm with you that we need to be careful here.  I am under the
impression though that there's no additional impact here because this is
just something that happens at compile-time.  Or if you load a source
file, but in that case you're evaluating and expecting a perf loss is
not the end of the world.

Andy

bug#19540: repeated ./././ in compiled modules

[Ludovic Courtès]

Andy Wingo <wingo@pobox.com> skribis:

> On Fri 24 Jun 2016 10:28, ludo@gnu.org (Ludovic Courtès) writes:
>
>> Hello!
>>
>> Andy Wingo <wingo@pobox.com> skribis:
>>
>>> On Thu 23 Jun 2016 15:06, ludo@gnu.org (Ludovic Courtès) writes:
>>>
>>>> ‘canonicalize_file_name’ is costly: roughly one syscall per file name
>>>> component.
>>>>
>>>> IIUC, ‘canonicalize_file_name’ is now called once for each ‘%load-path’
>>>> entry and file name that we canonicalize.  Is this correct?
>>>
>>> That's correct, but only for relative canonicalization, which is in
>>> practive only when loading Scheme files from source.  Seems out of the
>>> hot path; what do you think?
>>
>> I think it’s likely to have a noticeable impact on startup time for
>> projects with a large number of modules like Guix.
>
> Aren't they usually compiled?  Loading compiled files will not
> go through this path AFAIU.

Good point, I had overlooked that.

>> For instance, commands like ‘guix package -A’ or ‘guix build foo’ load
>> all the modules.  The impact will be smaller on a laptop with an SSD
>> than on an NFS mount, where it’s likely going to be terrrible (this
>> could be tested using the ‘delay’ device mapper.)
>
> Oh I'm with you that we need to be careful here.  I am under the
> impression though that there's no additional impact here because this is
> just something that happens at compile-time.  Or if you load a source
> file, but in that case you're evaluating and expecting a perf loss is
> not the end of the world.

Indeed, this makes sense to me now; sorry for the confusion!

Ludo’.