[PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)

[Mark H Weaver]

Hello all,

Here's a preliminary patch to avoid modifying LD_LIBRARY_PATH.

Following Bruce's suggestion, it causes 'sysdep_dynl_link' to manually
search additional directories if 'lt_dlopenext' fails to find the
library in the default paths.  However, I took a somewhat different
approach, and tried to be more careful with regard to portability and
correctness.

I read the code of libltdl, and mimicked their handling of path
separators and directory separators, to ensure that this patch will not
reduce our portability.

I also followed their lead in deciding when to perform a search.  If any
directory separators are present (even if it's a relative pathname),
then libltdl does not perform a search.  I used precisely the same
criterion to decide whether to search additional directories.

So what additional directories does it search?

If GUILE_SYSTEM_EXTENSIONS_PATH is set (even if it's empty), then it
specifies the additional directories to search.  If it's unset, then the
default is to search SCM_LIB_DIR and SCM_EXTENSIONS_DIR.

*** Note that this changes the search order in the case where
GUILE_SYSTEM_EXTENSIONS_PATH is set to a non-empty string.

Currently, a non-empty GUILE_SYSTEM_EXTENSIONS_PATH is passed to
lt_dladdsearchdir, so it is searched before LTDL_LIBRARY_PATH and
LD_LIBRARY_PATH, but this patch causes GUILE_SYSTEM_EXTENSIONS_PATH to
be searched last, to be consistent with the handling of the default
directories SCM_LIB_DIR and SCM_EXTENSIONS_DIR.  This seems sensible to
me.  Does anyone see a problem with this change?

This patch also adds robust handling of the case where
GUILE_SYSTEM_EXTENSIONS_PATH contains more than one path component.

See below for my preliminary patch.  I have not yet tested it carefully.
It's a context diff, because 'diff' made a mess of the unified diff.

Comments and suggestions solicited.

     Mark


diff --git a/libguile/dynl.c b/libguile/dynl.c
index a2ae6e2..149ed26 100644
*** a/libguile/dynl.c
--- b/libguile/dynl.c
***************
*** 26,31 ****
--- 26,33 ----
  #endif
  
  #include <alloca.h>
+ #include <assert.h>
+ #include <string.h>
  
  /* "dynl.c" dynamically link&load object files.
     Author: Aubrey Jaffer
***************
*** 37,43 ****
     solution would probably be a shared libgcc. */
  
  #undef NDEBUG
- #include <assert.h>
  
  static void
  maybe_drag_in_eprintf ()
--- 39,44 ----
***************
*** 75,92 ****
  */
  /* njrev: not threadsafe, protection needed as described above */
  
  static void *
  sysdep_dynl_link (const char *fname, const char *subr)
  {
    lt_dlhandle handle;
  
!   if (fname != NULL)
!     handle = lt_dlopenext (fname);
    else
!     /* Return a handle for the program as a whole.  */
!     handle = lt_dlopen (NULL);
  
!   if (NULL == handle)
      {
        SCM fn;
        SCM msg;
--- 76,165 ----
  */
  /* njrev: not threadsafe, protection needed as described above */
  
+ 
+ /* 'system_extensions_path' is used by 'sysdep_dynl_link' to search for
+    dynamic libraries as a last resort, when they cannot be found in the
+    usual library search paths. */
+ static char *system_extensions_path;
+ 
  static void *
  sysdep_dynl_link (const char *fname, const char *subr)
  {
    lt_dlhandle handle;
  
!   if (fname == NULL)
!     {
!       /* Return a handle for the program as a whole.  */
!       handle = lt_dlopen (NULL);
!     }
    else
!     {
!       handle = lt_dlopenext (fname);
! 
!       if (handle == NULL
! #ifdef LT_DIRSEP_CHAR
!           && strchr (fname, LT_DIRSEP_CHAR) == NULL
! #endif
!           && strchr (fname, '/') == NULL)
!         {
!           /* 'fname' contains no directory separators and was not in the
!              usual library search paths, so now we search for it in the
!              directories specified in 'system_extensions_path'. */
!           char *fname_attempt = malloc (strlen (system_extensions_path)
!                                         + strlen (fname)
!                                         + 1   /* for directory separator */
!                                         + 1); /* for null terminator */
!           char *path;  /* remaining path to search */
!           char *end;   /* end of current path component */
!           char *s;
! 
!           if (fname_attempt != NULL)
!             {
!               scm_dynwind_begin (0);
!               scm_dynwind_free (fname_attempt);
! 
!               /* Iterate over the components of 'system_extensions_path' */
!               for (path = system_extensions_path;
!                    *path != '\0';
!                    path = (*end == '\0') ? end : (end + 1))
!                 {
!                   /* Find end of pathname component */
!                   end = strchr (path, LT_PATHSEP_CHAR);
!                   if (end == NULL)
!                     end = strchr (path, '\0');
! 
!                   /* Skip empty path components */
!                   if (path == end)
!                     continue;
! 
!                   /* Construct 'fname_attempt', starting with path component 
*/
!                   s = fname_attempt;
!                   memcpy (s, path, end - path);
!                   s += end - path;
! 
!                   /* Append directory separator, but avoid duplicates */
!                   if (s[-1] != '/'
! #ifdef LT_DIRSEP_CHAR
!                       && s[-1] != LT_DIRSEP_CHAR
! #endif
!                       )
!                     *s++ = '/';
! 
!                   /* Finally, append 'fname' with null terminator */
!                   strcpy (s, fname);
! 
!                   /* Try to load it, and terminate the search if successful */
!                   handle = lt_dlopenext (fname_attempt);
!                   if (handle != NULL)
!                     break;
!                 }
! 
!               scm_dynwind_end ();
!             }
!         }
!     }
  
!   if (handle == NULL)
      {
        SCM fn;
        SCM msg;
***************
*** 120,149 ****
    return fptr;
  }
  
- /* Augment environment variable VARIABLE with VALUE, assuming VARIABLE
-    is a path kind of variable.  */
- static void
- augment_env (const char *variable, const char *value)
- {
-   const char *env;
- 
-   env = getenv (variable);
-   if (env != NULL)
-     {
-       char *new_value;
-       static const char path_sep[] = { LT_PATHSEP_CHAR, 0 };
- 
-       new_value = alloca (strlen (env) + strlen (value) + 2);
-       strcpy (new_value, env);
-       strcat (new_value, path_sep);
-       strcat (new_value, value);
- 
-       setenv (variable, new_value, 1);
-     }
-   else
-     setenv (variable, value, 1);
- }
- 
  static void
  sysdep_dynl_init ()
  {
--- 193,198 ----
***************
*** 151,176 ****
  
    lt_dlinit ();
  
    env = getenv ("GUILE_SYSTEM_EXTENSIONS_PATH");
!   if (env && strcmp (env, "") == 0)
!     /* special-case interpret system-ltdl-path=="" as meaning no system path,
!        which is the case during the build */
!     ;
!   else if (env)
!     /* FIXME: should this be a colon-separated path? Or is the only point to
!        allow the build system to turn off the installed extensions path? */
!     lt_dladdsearchdir (env);
    else
      {
!       /* Add SCM_LIB_DIR and SCM_EXTENSIONS_DIR to the loader's search
!        path.  `lt_dladdsearchdir' and $LTDL_LIBRARY_PATH can't be used
!        for that because they are searched before the system-dependent
!        search path, which is the one `libtool --mode=execute -dlopen'
!        fiddles with (info "(libtool) Libltdl Interface").  See
!        <http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html>
!        for details.  */
!       augment_env (SHARED_LIBRARY_PATH_VARIABLE, SCM_LIB_DIR);
!       augment_env (SHARED_LIBRARY_PATH_VARIABLE, SCM_EXTENSIONS_DIR);
      }
  }
  
--- 200,232 ----
  
    lt_dlinit ();
  
+   /* Initialize 'system_extensions_path' from
+      $GUILE_SYSTEM_EXTENSIONS_PATH, or if that's not set:
+      <SCM_LIB_DIR> <LT_PATHSEP_CHAR> <SCM_EXTENSIONS_DIR>.
+ 
+      'lt_dladdsearchdir' can't be used because it is searched before the
+      system-dependent search path, which is the one 'libtool
+      --mode=execute -dlopen' fiddles with (info "(libtool) Libltdl
+      Interface").  See
+      <http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html>.
+ 
+      The environment variables $LTDL_LIBRARY_PATH and $LD_LIBRARY_PATH
+      can't be used because they would be propagated to subprocesses
+      which may cause problems for other programs.  See
+      <http://lists.gnu.org/archive/html/guile-devel/2012-09/msg00037.html> */
+ 
    env = getenv ("GUILE_SYSTEM_EXTENSIONS_PATH");
!   if (env)
!     system_extensions_path = env;
    else
      {
!       system_extensions_path = (char *) malloc (strlen (SCM_LIB_DIR)
!                                                 + strlen (SCM_EXTENSIONS_DIR)
!                                                 + 1   /* for path separator */
!                                                 + 1); /* for null terminator 
*/
!       assert (system_extensions_path != NULL);
!       sprintf (system_extensions_path, "%s%c%s",
!                SCM_LIB_DIR, LT_PATHSEP_CHAR, SCM_EXTENSIONS_DIR);
      }
  }