bug#18835: load-from-path is inconsistent when looking for a compiled version of the source file

[Taylan Kammer <taylan.kammer@gmail.com>]

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

In 2016, Andy wrote:

> The logic in load.c is that we only add on .go if the file
> doesn't already have an extension.  If the file has an extension and
> it's not .go, then we don't grovel in the path at all.  I guess this is
> the wrong thing?
> 
> I am not sure if we can change this in 2.0 or not.  I guess we can.

Well that ship has sailed anyway. ;-)

IMO we should change the behavior to always try to add extensions, after
first trying without an extension.

It's feasible that one might choose to name one's files like foo.bar.scm
and the like, in which case trying to load foo.bar should work.  I might
name a number of files foo.v1.scm, foo.v2.scm, and so on.  Alternatively,
consider the already popular foo.upstream.scm.

Further, I don't see any possible confusion arising from adding the
extensions.  No one would name their files foo.go.scm or foo.scm.scm and
then try to load "foo.go" or "foo.scm" and expect the one with an extra
.scm to be loaded.

I thought a bit whether there might be security implications, like when
dropping files into a directory where every user can create files, and
someone could maliciously put a foo.scm.scm to take precedence over your
foo.scm, but that's already a problem as they could drop in a foo.go, so
the solution is not to try to load files from directories you can't trust,
like /tmp.

The only compatibility issue I can think of: maybe some people put both a
file foo and a file foo.scm in the same directory, and expect (load "foo")
to *not* try to load the one without an extension.

Thoughts?


Attached is a patch that would implement the suggestion of trying without
an extension first, then with an extension, without checking whether the
original filename does or doesn't have an extension.

-- 
Taylan

[-- Attachment #2: 0001-Make-load-try-to-add-an-extension-to-files-that-alre.patch --]
[-- Type: text/plain, Size: 4640 bytes --]

From 2ed80f831006815e37374a74218af4572703229f Mon Sep 17 00:00:00 2001
From: Taylan Kammer <taylan.kammer@gmail.com>
Date: Mon, 17 May 2021 19:31:40 +0200
Subject: [PATCH] Make load try to add an extension to files that already have
 one.

* libguile/load.c (load_thunk_from_path, search_path): Remove the code
that checks whether the filename already has an extension.  Add the
empty string to the head of the extensions list.
---
 libguile/load.c | 67 +++++--------------------------------------------
 1 file changed, 6 insertions(+), 61 deletions(-)

diff --git a/libguile/load.c b/libguile/load.c
index e95c36db1..0c198f165 100644
--- a/libguile/load.c
+++ b/libguile/load.c
@@ -649,7 +649,6 @@ load_thunk_from_path (SCM filename, SCM source_file_name,
   struct stringbuf buf;
   struct stat stat_buf;
   char *filename_chars;
-  size_t filename_len;
   SCM path, extensions;
   SCM result = SCM_BOOL_F;
   char initial_buffer[256];
@@ -667,7 +666,6 @@ load_thunk_from_path (SCM filename, SCM source_file_name,
   scm_dynwind_begin (0);
 
   filename_chars = scm_to_locale_string (filename);
-  filename_len = strlen (filename_chars);
   scm_dynwind_free (filename_chars);
 
   /* If FILENAME is absolute and is still valid, return it unchanged.  */
@@ -680,34 +678,6 @@ load_thunk_from_path (SCM filename, SCM source_file_name,
       goto end;
     }
 
-  /* If FILENAME has an extension, don't try to add EXTENSIONS to it.  */
-  {
-    char *endp;
-
-    for (endp = filename_chars + filename_len - 1;
-	 endp >= filename_chars;
-	 endp--)
-      {
-	if (*endp == '.')
-	  {
-            if (!string_has_an_ext (filename, extensions))
-              {
-                /* This filename has an extension, but not one of the right
-                   ones... */
-                goto end;
-              }
-	    /* This filename already has an extension, so cancel the
-               list of extensions.  */
-	    extensions = SCM_EOL;
-	    break;
-	  }
-	else if (is_file_name_separator (SCM_MAKE_CHAR (*endp)))
-	  /* This filename has no extension, so keep the current list
-             of extensions.  */
-	  break;
-      }
-  }
-
   /* This simplifies the loop below a bit.
    */
   if (scm_is_null (extensions))
@@ -736,6 +706,9 @@ load_thunk_from_path (SCM filename, SCM source_file_name,
       stringbuf_cat (&buf, filename_chars);
       sans_ext_len = buf.ptr - buf.buf;
 
+      /* Add the empty string as the first "extension." */
+      extensions = scm_cons (scm_nullstr, extensions);
+
       /* Try every extension. */
       for (exts = extensions; scm_is_pair (exts); exts = SCM_CDR (exts))
 	{
@@ -805,7 +778,6 @@ search_path (SCM path, SCM filename, SCM extensions, SCM require_exts,
 {
   struct stringbuf buf;
   char *filename_chars;
-  size_t filename_len;
   SCM result = SCM_BOOL_F;
   char initial_buffer[256];
 
@@ -819,7 +791,6 @@ search_path (SCM path, SCM filename, SCM extensions, SCM require_exts,
   scm_dynwind_begin (0);
 
   filename_chars = scm_to_locale_string (filename);
-  filename_len = strlen (filename_chars);
   scm_dynwind_free (filename_chars);
 
   /* If FILENAME is absolute and is still valid, return it unchanged.  */
@@ -833,35 +804,6 @@ search_path (SCM path, SCM filename, SCM extensions, SCM require_exts,
       goto end;
     }
 
-  /* If FILENAME has an extension, don't try to add EXTENSIONS to it.  */
-  {
-    char *endp;
-
-    for (endp = filename_chars + filename_len - 1;
-	 endp >= filename_chars;
-	 endp--)
-      {
-	if (*endp == '.')
-	  {
-            if (scm_is_true (require_exts) &&
-                !string_has_an_ext (filename, extensions))
-              {
-                /* This filename has an extension, but not one of the right
-                   ones... */
-                goto end;
-              }
-	    /* This filename already has an extension, so cancel the
-               list of extensions.  */
-	    extensions = SCM_EOL;
-	    break;
-	  }
-	else if (is_file_name_separator (SCM_MAKE_CHAR (*endp)))
-	  /* This filename has no extension, so keep the current list
-             of extensions.  */
-	  break;
-      }
-  }
-
   /* This simplifies the loop below a bit.
    */
   if (scm_is_null (extensions))
@@ -890,6 +832,9 @@ search_path (SCM path, SCM filename, SCM extensions, SCM require_exts,
       stringbuf_cat (&buf, filename_chars);
       sans_ext_len = buf.ptr - buf.buf;
 
+      /* Add the empty string as the first "extension." */
+      extensions = scm_cons (scm_nullstr, extensions);
+
       /* Try every extension. */
       for (exts = extensions; scm_is_pair (exts); exts = SCM_CDR (exts))
 	{
-- 
2.30.2