diff mbox series

[1/1] elf: Canonicalize $ORIGIN in an explicit ld.so invocation [BZ #25263]

Message ID 20250218205816.60861-1-geofft@ldpreload.com (mailing list archive)
State Superseded
Delegated to: Adhemerval Zanella Netto
Headers
Series [1/1] elf: Canonicalize $ORIGIN in an explicit ld.so invocation [BZ #25263] |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed

Commit Message

Geoffrey Thomas Feb. 18, 2025, 8:58 p.m. UTC 
  When an executable is invoked directly, we calculate $ORIGIN by calling
readlink on /proc/self/exe, which the Linux kernel resolves to the
target of any symlinks. However, if an executable is run through ld.so,
we cannot use /proc/self/exe and instead use the path given as an
argument. This leads to a different calculation of $ORIGIN, which is
most notable in that it causes ldd to behave differently (e.g., by not
finding a library) from directly running the program.

To make the behavior consistent, take advantage of the fact that the
kernel also resolves /proc/self/fd/ symlinks to the target of any
symlinks in the same manner, so once we have opened the main executable
in order to load it, replace the user-provided path with the result of
calling readlink("/proc/self/fd/N").

(On non-Linux platforms this resolution does not happen and so no
behavior change is needed.)
---
 elf/dl-load.c                       |  1 +
 elf/dl-origin.c                     |  5 +++++
 sysdeps/generic/ldsodefs.h          |  5 +++++
 sysdeps/unix/sysv/linux/dl-origin.c | 27 +++++++++++++++++++++++++++
 4 files changed, 38 insertions(+)

Comments

Adhemerval Zanella Feb. 27, 2025, 8:26 p.m. UTC | #1 
On 18/02/25 17:58, Geoffrey Thomas wrote:
> When an executable is invoked directly, we calculate $ORIGIN by calling
> readlink on /proc/self/exe, which the Linux kernel resolves to the
> target of any symlinks. However, if an executable is run through ld.so,
> we cannot use /proc/self/exe and instead use the path given as an
> argument. This leads to a different calculation of $ORIGIN, which is
> most notable in that it causes ldd to behave differently (e.g., by not
> finding a library) from directly running the program.
> 
> To make the behavior consistent, take advantage of the fact that the
> kernel also resolves /proc/self/fd/ symlinks to the target of any
> symlinks in the same manner, so once we have opened the main executable
> in order to load it, replace the user-provided path with the result of
> calling readlink("/proc/self/fd/N").
> 
> (On non-Linux platforms this resolution does not happen and so no
> behavior change is needed.)

Although this change solves the LD_TRACE_LOADED_OBJECTS=1 (ldd) issue, I
think it still misses the underlying issue for the case user want to also 
run the binary with a direct loader invocation (as Florian has hinted on 
comment #4).

For instance, with this testcase (similar to what electron is doing from
the bug report):

$ cat test/main.c
int main (int argc, char *argv[]) { return 0; }
$ cat test/lib1.c
int foo (void) { return 42; }
tst-bz25263 $ mkdir test && cd test
test $ gcc -Wall -shared -Wl,-soname,lib1.so lib1.c -o lib1.so
test $ gcc -no-pie -Wall main.c -o main -Wl,-rpath,\$ORIGIN -L`pwd` -Wl,-no-as-needed -l1
test $ gcc -no-pie -Wall main.c -o main-hcp -Wl,-rpath,\$ORIGIN:$GLIBCBUILDDIR -Wl,-dynamic-linker,$GLIBCBUILDDIR/elf/ld.so -L`pwd` -Wl,-no-as-needed -l1
main $ cd ..
$ ln -s test/main main
$ ln -s test/main-hcp main-hcp

So 'main' is a binary without just one RUNPATH with $ORIGIN, while 'main-hcp'
is binary that will use a built glibc (along with the RUNPATH).

Checking the library path resolution for both cases:

$ LD_DEBUG=libs ./main-hcp
   3151277:     find library=lib1.so [0]; searching
   3151277:      search path=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v3:/home/azanella/Projects/glibc/build/
x86_64-linux-gnu-work/tst-bz25263/test/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR/tst-bz25263/test:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR          (RUNPATH from file ./main-hcp)
   3151277:       trying file=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v3/lib1.so
   3151277:       trying file=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v2/lib1.so
   3151277:       trying file=$GLIBCBUILDDIR/tst-bz25263/test/lib1.so

LD_DEBUG=libs $GLIBCBUILDDIR/elf/ld.so --library-path $GLIBCBUILDDIR ./main
   3151286:	find library=lib1.so [0]; searching
   3151286:	 search path=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR		(--library-path)
   3151286:	  trying file=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3/lib1.so
   3151286:	  trying file=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2/lib1.so
   3151286:	  trying file=$GLIBCBUILDDIR/lib1.so
   3151286:	 search path=$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR/tst-bz25263/.		(RUNPATH from file ./main)
   3151286:	  trying file=$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v3/lib1.so
   3151286:	  trying file=$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v2/lib1.so
   3151286:	  trying file=$GLIBCBUILDDIR/tst-bz25263/./lib1.so
   3151286:	 search cache=/etc/ld.so.cache
   3151286:	 search path=/lib64/glibc-hwcaps/x86-64-v3:/lib64/glibc-hwcaps/x86-64-v2:/lib64:/usr/lib64/glibc-hwcaps/x86-64-v3:/usr/lib64/glibc-hwcaps/x86-64-v2:/usr/lib64		(system search path)
   3151286:	  trying file=/lib64/glibc-hwcaps/x86-64-v3/lib1.so
   3151286:	  trying file=/lib64/glibc-hwcaps/x86-64-v2/lib1.so
   3151286:	  trying file=/lib64/lib1.so
   3151286:	  trying file=/usr/lib64/glibc-hwcaps/x86-64-v3/lib1.so
   3151286:	  trying file=/usr/lib64/glibc-hwcaps/x86-64-v2/lib1.so
   3151286:	  trying file=/usr/lib64/lib1.so

You can see that if the binary is not issued directly by the loader (main-hcp),
it will have the full realpath on the search path (GLIBCBUILDDIR/tst-bz25263/test);
different that binary issued by the loader which is has the current directory
(GLIBCBUILDDIR/tst-bz25263).

And I think the main issue is how we initialize the link_map on _dl_new_object,
where l_origin is set to current directly where it should be the realpath from 
the realname.

Something like:

diff --git a/elf/dl-object.c b/elf/dl-object.c
index 51d3704edc..0288d049d4 100644
--- a/elf/dl-object.c
+++ b/elf/dl-object.c
@@ -24,7 +24,6 @@

 #include <assert.h>

-
 /* Add the new link_map NEW to the end of the namespace list.  */
 void
 _dl_add_to_namespace_list (struct link_map *new, Lmid_t nsid)
@@ -189,7 +188,7 @@ _dl_new_object (char *realname, const char *libname, int type,
   if (realname[0] != '\0')
     {
       size_t realname_len = strlen (realname) + 1;
-      char *origin;
+      char *origin = NULL;
       char *cp;

       if (realname[0] == '/')
@@ -202,48 +201,21 @@ _dl_new_object (char *realname, const char *libname, int type,
              origin = (char *) -1;
              goto out;
            }
+
+         /* Add the real file name.  */
+         cp = __mempcpy (cp, realname, realname_len);
        }
       else
        {
-         size_t len = realname_len;
-         char *result = NULL;
-
-         /* Get the current directory name.  */
-         origin = NULL;
-         do
-           {
-             char *new_origin;
-
-             len += 128;
-             new_origin = (char *) realloc (origin, len);
-             if (new_origin == NULL)
-               /* We exit the loop.  Note that result == NULL.  */
-               break;
-             origin = new_origin;
-           }
-         while ((result = __getcwd (origin, len - realname_len)) == NULL
-                && errno == ERANGE);
-
+         char *result = realpath_test (realname, NULL);
          if (result == NULL)
-           {
-             /* We were not able to determine the current directory.
-                Note that free(origin) is OK if origin == NULL.  */
-             free (origin);
-             origin = (char *) -1;
-             goto out;
-           }
-
-         /* Find the end of the path and see whether we have to add a
-            slash.  We could use rawmemchr but this need not be
-            fast.  */
-         cp = (strchr) (origin, '\0');
+           goto out;
+         origin = dirname_test (result);
+         cp = (strchr) (result, '\0');
          if (cp[-1] != '/')
            *cp++ = '/';
        }

-      /* Add the real file name.  */
-      cp = __mempcpy (cp, realname, realname_len);
-
       /* Now remove the filename and the slash.  Leave the slash if
         the name is something like "/foo".  */
       do

I have to use some mockup implementation for realpath_test and 
dirname_test because canocanlize is quite complex and it pulls too
much code. But the idea is have the same semantic as the symbol provided
by the libc (it would most likely have to change the stdlib/canonicalize.c
to meet the loader requirements).

With this patch applied, I see now:

$ LD_DEBUG=libs $GLIBCBUILDDIR/elf/ld.so --library-path $GLIBCBUILDDIR ./main
   3159143:	decompose_rpath
   3159143:	find library=lib1.so [0]; searching
   3159143:	 search path=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR		(--library-path)
   3159143:	  trying file=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3/lib1.so
   3159143:	  trying file=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2/lib1.so
   3159143:	  trying file=$GLIBCBUILDDIR/lib1.so
   3159143:	 search path=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR/tst-bz25263/test		(RUNPATH from file ./main)
   3159143:	  trying file=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v3/lib1.so
   3159143:	  trying file=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v2/lib1.so
   3159143:	  trying file=$GLIBCBUILDDIR/tst-bz25263/test/lib1.so

So I think we should move to a fix that works for the loader not only
for LD_TRACE_LOADED_OBJECTS. 

Besides this, this issue would also require a proper regression test.

> ---
>  elf/dl-load.c                       |  1 +
>  elf/dl-origin.c                     |  5 +++++
>  sysdeps/generic/ldsodefs.h          |  5 +++++
>  sysdeps/unix/sysv/linux/dl-origin.c | 27 +++++++++++++++++++++++++++
>  4 files changed, 38 insertions(+)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index f905578a65..2ef585bc5c 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -965,6 +965,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>      {
>        assert (nsid == LM_ID_BASE);
>        memset (&id, 0, sizeof (id));
> +      _dl_canonicalize (&realname, fd);
>      }
>    else
>      {
> diff --git a/elf/dl-origin.c b/elf/dl-origin.c
> index 9f6b921b01..d942f3f0bb 100644
> --- a/elf/dl-origin.c
> +++ b/elf/dl-origin.c
> @@ -47,3 +47,8 @@ _dl_get_origin (void)
>  
>    return result;
>  }
> +
> +void
> +_dl_canonicalize (char **filename, int fd)
> +{
> +}
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index e871f27ff2..36b472e19c 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -1198,6 +1198,11 @@ extern struct link_map * _dl_get_dl_main_map (void) attribute_hidden;
>  /* Find origin of the executable.  */
>  extern const char *_dl_get_origin (void) attribute_hidden;
>  
> +/* Canonicalize the path to an open file.  FILENAME is a pointer to a
> +   string allocated with malloc; it may be freed and replaced with
> +   another string allocated with malloc. */
> +extern void _dl_canonicalize (char **filename, int fd) attribute_hidden;
> +
>  /* Count DSTs.  */
>  extern size_t _dl_dst_count (const char *name) attribute_hidden;
>  
> diff --git a/sysdeps/unix/sysv/linux/dl-origin.c b/sysdeps/unix/sysv/linux/dl-origin.c
> index decdd8ae9e..b12eff4315 100644
> --- a/sysdeps/unix/sysv/linux/dl-origin.c
> +++ b/sysdeps/unix/sysv/linux/dl-origin.c
> @@ -72,3 +72,30 @@ _dl_get_origin (void)
>  
>    return result;
>  }
> +
> +/* On Linux, readlink on the magic symlinks in /proc/self/fd also has
> +   the same behavior of returning the canonical path from the dcache.
> +   If it does not work, we do not bother to canonicalize. */
> +
> +void
> +_dl_canonicalize (char **filename, int fd)
> +{
> +  char *canonical = (char *) malloc (PATH_MAX + 1);
> +  char buf[25];
> +  buf[24] = '\0';
> +  char *path = _itoa (fd, buf + 24, 10, 0);
> +  path = memcpy (path - 14, "/proc/self/fd/", 14);
> +
> +  int size = INTERNAL_SYSCALL_CALL (readlinkat, AT_FDCWD, path,
> +				    canonical, PATH_MAX );
> +  if (size >= 0)
> +    {
> +      free (*filename);
> +      canonical[size] = '\0';
> +      *filename = canonical;
> +    }
> +  else
> +    {
> +      free (canonical);
> +    }
> +}
Geoffrey Thomas Feb. 28, 2025, 7:02 p.m. UTC | #2 
On Thu, Feb 27, 2025, at 3:26 PM, Adhemerval Zanella Netto wrote:
> Although this change solves the LD_TRACE_LOADED_OBJECTS=1 (ldd) issue, I
> think it still misses the underlying issue for the case user want to also 
> run the binary with a direct loader invocation (as Florian has hinted on 
> comment #4).
>
> For instance, with this testcase (similar to what electron is doing from
> the bug report):
>
> $ cat test/main.c
> int main (int argc, char *argv[]) { return 0; }
> $ cat test/lib1.c
> int foo (void) { return 42; }
> tst-bz25263 $ mkdir test && cd test
> test $ gcc -Wall -shared -Wl,-soname,lib1.so lib1.c -o lib1.so
> test $ gcc -no-pie -Wall main.c -o main -Wl,-rpath,\$ORIGIN -L`pwd` 
> -Wl,-no-as-needed -l1
> test $ gcc -no-pie -Wall main.c -o main-hcp 
> -Wl,-rpath,\$ORIGIN:$GLIBCBUILDDIR 
> -Wl,-dynamic-linker,$GLIBCBUILDDIR/elf/ld.so -L`pwd` -Wl,-no-as-needed 
> -l1
> main $ cd ..
> $ ln -s test/main main
> $ ln -s test/main-hcp main-hcp
>
> So 'main' is a binary without just one RUNPATH with $ORIGIN, while 'main-hcp'
> is binary that will use a built glibc (along with the RUNPATH).
>
> Checking the library path resolution for both cases:
>
> $ LD_DEBUG=libs ./main-hcp
>    3151277:     find library=lib1.so [0]; searching
>    3151277:      search 
> path=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v3:/home/azanella/Projects/glibc/build/
> x86_64-linux-gnu-work/tst-bz25263/test/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR/tst-bz25263/test:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR 
>          (RUNPATH from file ./main-hcp)
>    3151277:       trying 
> file=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v3/lib1.so
>    3151277:       trying 
> file=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v2/lib1.so
>    3151277:       trying file=$GLIBCBUILDDIR/tst-bz25263/test/lib1.so
>
> LD_DEBUG=libs $GLIBCBUILDDIR/elf/ld.so --library-path $GLIBCBUILDDIR 
> ./main
>    3151286:	find library=lib1.so [0]; searching
>    3151286:	 search 
> path=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR		(--library-path)
>    3151286:	  trying file=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3/lib1.so
>    3151286:	  trying file=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2/lib1.so
>    3151286:	  trying file=$GLIBCBUILDDIR/lib1.so
>    3151286:	 search 
> path=$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR/tst-bz25263/.		(RUNPATH 
> from file ./main)
>    3151286:	  trying 
> file=$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v3/lib1.so
>    3151286:	  trying 
> file=$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v2/lib1.so
>    3151286:	  trying file=$GLIBCBUILDDIR/tst-bz25263/./lib1.so
>    3151286:	 search cache=/etc/ld.so.cache
>    3151286:	 search 
> path=/lib64/glibc-hwcaps/x86-64-v3:/lib64/glibc-hwcaps/x86-64-v2:/lib64:/usr/lib64/glibc-hwcaps/x86-64-v3:/usr/lib64/glibc-hwcaps/x86-64-v2:/usr/lib64		(system 
> search path)
>    3151286:	  trying file=/lib64/glibc-hwcaps/x86-64-v3/lib1.so
>    3151286:	  trying file=/lib64/glibc-hwcaps/x86-64-v2/lib1.so
>    3151286:	  trying file=/lib64/lib1.so
>    3151286:	  trying file=/usr/lib64/glibc-hwcaps/x86-64-v3/lib1.so
>    3151286:	  trying file=/usr/lib64/glibc-hwcaps/x86-64-v2/lib1.so
>    3151286:	  trying file=/usr/lib64/lib1.so
>
> You can see that if the binary is not issued directly by the loader 
> (main-hcp),
> it will have the full realpath on the search path 
> (GLIBCBUILDDIR/tst-bz25263/test);
> different that binary issued by the loader which is has the current 
> directory
> (GLIBCBUILDDIR/tst-bz25263).

This is with my patch? Directly on top of glibc-2.41 (commit db71c940
from github.com/geofft/glibc), with a checkout at /glibc and so
GLIBCBUILDDIR=/glibc/build, I get

[root@nixos:/glibc/build/tst-bz25263]# ls -l
total 0
lrwxr-xr-x 1 root root   9 Feb 28 13:37 main -> test/main
lrwxr-xr-x 1 root root  13 Feb 28 13:37 main-hcp -> test/main-hcp
drwxr-xr-x 7 root root 224 Feb 28 13:37 test
[root@nixos:/glibc/build/tst-bz25263]# readelf -d main | grep NEEDED
 0x0000000000000001 (NEEDED)             Shared library: [lib1.so]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
[root@nixos:/glibc/build/tst-bz25263]# LD_DEBUG=libs /glibc/build/elf/ld.so --library-path /glibc/build ./main
     49044:     find library=lib1.so [0]; searching
     49044:      search path=/glibc/build               (--library-path)
     49044:       trying file=/glibc/build/lib1.so
     49044:      search path=/glibc/build/tst-bz25263/test:/nix/store/6j2qcdi8axcl91v08blc3vjn39y117z3-shell/lib:/nix/store/q6kvdfhlp251n8rx183lg1n245kwg6m8-glibc-2.39-52/lib:/nix/store/fxh2cg0sfgygfya6qjr11gzj4sbjx0h9-gcc-13.2.0-lib/lib         (RUNPATH from file ./main)
     49044:       trying file=/glibc/build/tst-bz25263/test/lib1.so
     49044:
     49044:     find library=libc.so.6 [0]; searching
     49044:      search path=/glibc/build               (--library-path)
     49044:       trying file=/glibc/build/libc.so.6
[...]

which, if I'm following correctly, is the fixed behavior.

As expected it does not work with a copy instead of a symlink:

[root@nixos:/glibc/build/tst-bz25263]# cp test/main maincopy
[root@nixos:/glibc/build/tst-bz25263]# LD_DEBUG=libs /glibc/build/elf/ld.so --library-path /glibc/build ./maincopy
     49073:     find library=lib1.so [0]; searching
     49073:      search path=/glibc/build               (--library-path)
     49073:       trying file=/glibc/build/lib1.so
     49073:      search path=/glibc/build/tst-bz25263:/nix/store/6j2qcdi8axcl91v08blc3vjn39y117z3-shell/lib:/nix/store/q6kvdfhlp251n8rx183lg1n245kwg6m8-glibc-2.39-52/lib:/nix/store/fxh2cg0sfgygfya6qjr11gzj4sbjx0h9-gcc-13.2.0-lib/lib              (RUNPATH from file ./maincopy)
     49073:       trying file=/glibc/build/tst-bz25263/lib1.so
     49073:       trying file=/nix/store/6j2qcdi8axcl91v08blc3vjn39y117z3-shell/lib/lib1.so
     49073:       trying file=/nix/store/q6kvdfhlp251n8rx183lg1n245kwg6m8-glibc-2.39-52/lib/lib1.so
     49073:       trying file=/nix/store/fxh2cg0sfgygfya6qjr11gzj4sbjx0h9-gcc-13.2.0-lib/lib/lib1.so
     49073:      search cache=/root/etc/ld.so.cache
     49073:      search path=/root/lib          (system search path)
     49073:       trying file=/root/lib/lib1.so
     49073:
./maincopy: error while loading shared libraries: lib1.so: cannot open shared object file: No such file or directory

What commit and OS and arch are you on? (It's possible I'm confusing 
myself by using NixOS, maybe.)
Adhemerval Zanella March 1, 2025, 1:27 a.m. UTC | #3 
On 28/02/25 16:02, Geoffrey Thomas wrote:
> On Thu, Feb 27, 2025, at 3:26 PM, Adhemerval Zanella Netto wrote:
>> Although this change solves the LD_TRACE_LOADED_OBJECTS=1 (ldd) issue, I
>> think it still misses the underlying issue for the case user want to also 
>> run the binary with a direct loader invocation (as Florian has hinted on 
>> comment #4).
>>
>> For instance, with this testcase (similar to what electron is doing from
>> the bug report):
>>
>> $ cat test/main.c
>> int main (int argc, char *argv[]) { return 0; }
>> $ cat test/lib1.c
>> int foo (void) { return 42; }
>> tst-bz25263 $ mkdir test && cd test
>> test $ gcc -Wall -shared -Wl,-soname,lib1.so lib1.c -o lib1.so
>> test $ gcc -no-pie -Wall main.c -o main -Wl,-rpath,\$ORIGIN -L`pwd` 
>> -Wl,-no-as-needed -l1
>> test $ gcc -no-pie -Wall main.c -o main-hcp 
>> -Wl,-rpath,\$ORIGIN:$GLIBCBUILDDIR 
>> -Wl,-dynamic-linker,$GLIBCBUILDDIR/elf/ld.so -L`pwd` -Wl,-no-as-needed 
>> -l1
>> main $ cd ..
>> $ ln -s test/main main
>> $ ln -s test/main-hcp main-hcp
>>
>> So 'main' is a binary without just one RUNPATH with $ORIGIN, while 'main-hcp'
>> is binary that will use a built glibc (along with the RUNPATH).
>>
>> Checking the library path resolution for both cases:
>>
>> $ LD_DEBUG=libs ./main-hcp
>>    3151277:     find library=lib1.so [0]; searching
>>    3151277:      search 
>> path=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v3:/home/azanella/Projects/glibc/build/
>> x86_64-linux-gnu-work/tst-bz25263/test/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR/tst-bz25263/test:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR 
>>          (RUNPATH from file ./main-hcp)
>>    3151277:       trying 
>> file=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v3/lib1.so
>>    3151277:       trying 
>> file=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v2/lib1.so
>>    3151277:       trying file=$GLIBCBUILDDIR/tst-bz25263/test/lib1.so
>>
>> LD_DEBUG=libs $GLIBCBUILDDIR/elf/ld.so --library-path $GLIBCBUILDDIR 
>> ./main
>>    3151286:	find library=lib1.so [0]; searching
>>    3151286:	 search 
>> path=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR		(--library-path)
>>    3151286:	  trying file=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3/lib1.so
>>    3151286:	  trying file=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2/lib1.so
>>    3151286:	  trying file=$GLIBCBUILDDIR/lib1.so
>>    3151286:	 search 
>> path=$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR/tst-bz25263/.		(RUNPATH 
>> from file ./main)
>>    3151286:	  trying 
>> file=$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v3/lib1.so
>>    3151286:	  trying 
>> file=$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v2/lib1.so
>>    3151286:	  trying file=$GLIBCBUILDDIR/tst-bz25263/./lib1.so
>>    3151286:	 search cache=/etc/ld.so.cache
>>    3151286:	 search 
>> path=/lib64/glibc-hwcaps/x86-64-v3:/lib64/glibc-hwcaps/x86-64-v2:/lib64:/usr/lib64/glibc-hwcaps/x86-64-v3:/usr/lib64/glibc-hwcaps/x86-64-v2:/usr/lib64		(system 
>> search path)
>>    3151286:	  trying file=/lib64/glibc-hwcaps/x86-64-v3/lib1.so
>>    3151286:	  trying file=/lib64/glibc-hwcaps/x86-64-v2/lib1.so
>>    3151286:	  trying file=/lib64/lib1.so
>>    3151286:	  trying file=/usr/lib64/glibc-hwcaps/x86-64-v3/lib1.so
>>    3151286:	  trying file=/usr/lib64/glibc-hwcaps/x86-64-v2/lib1.so
>>    3151286:	  trying file=/usr/lib64/lib1.so
>>
>> You can see that if the binary is not issued directly by the loader 
>> (main-hcp),
>> it will have the full realpath on the search path 
>> (GLIBCBUILDDIR/tst-bz25263/test);
>> different that binary issued by the loader which is has the current 
>> directory
>> (GLIBCBUILDDIR/tst-bz25263).
> 
> This is with my patch? Directly on top of glibc-2.41 (commit db71c940
> from github.com/geofft/glibc), with a checkout at /glibc and so
> GLIBCBUILDDIR=/glibc/build, I get

Yes, this your patch on top of 9e51ae3cd0c7f65bdeba93b7f1d780cdb21fc269

> 
> [root@nixos:/glibc/build/tst-bz25263]# ls -l
> total 0
> lrwxr-xr-x 1 root root   9 Feb 28 13:37 main -> test/main
> lrwxr-xr-x 1 root root  13 Feb 28 13:37 main-hcp -> test/main-hcp
> drwxr-xr-x 7 root root 224 Feb 28 13:37 test
> [root@nixos:/glibc/build/tst-bz25263]# readelf -d main | grep NEEDED
>  0x0000000000000001 (NEEDED)             Shared library: [lib1.so]
>  0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
> [root@nixos:/glibc/build/tst-bz25263]# LD_DEBUG=libs /glibc/build/elf/ld.so --library-path /glibc/build ./main
>      49044:     find library=lib1.so [0]; searching
>      49044:      search path=/glibc/build               (--library-path)
>      49044:       trying file=/glibc/build/lib1.so
>      49044:      search path=/glibc/build/tst-bz25263/test:/nix/store/6j2qcdi8axcl91v08blc3vjn39y117z3-shell/lib:/nix/store/q6kvdfhlp251n8rx183lg1n245kwg6m8-glibc-2.39-52/lib:/nix/store/fxh2cg0sfgygfya6qjr11gzj4sbjx0h9-gcc-13.2.0-lib/lib         (RUNPATH from file ./main)
>      49044:       trying file=/glibc/build/tst-bz25263/test/lib1.so
>      49044:
>      49044:     find library=libc.so.6 [0]; searching
>      49044:      search path=/glibc/build               (--library-path)
>      49044:       trying file=/glibc/build/libc.so.6
> [...]
> 
> which, if I'm following correctly, is the fixed behavior.

Sigh, for some reason I did not use git-pw on your patch and applied
directly from mailbox it was malformated with the wrong path for the
procfs. With 'git-pw patch apply 106751' it now seems to work.

You might want to use this instead (we have a helper to access the
procfs).

  struct fd_to_filename fdfilename;
  char *canonical = (char *) malloc (PATH_MAX + 1);
  char *path = __fd_to_filename (fd, &fdfilename);
  int size = INTERNAL_SYSCALL_CALL (readlinkat, AT_FDCWD, path,
                                    canonical, PATH_MAX);

It does seems to work.

> 
> As expected it does not work with a copy instead of a symlink:
> 
> [root@nixos:/glibc/build/tst-bz25263]# cp test/main maincopy
> [root@nixos:/glibc/build/tst-bz25263]# LD_DEBUG=libs /glibc/build/elf/ld.so --library-path /glibc/build ./maincopy
>      49073:     find library=lib1.so [0]; searching
>      49073:      search path=/glibc/build               (--library-path)
>      49073:       trying file=/glibc/build/lib1.so
>      49073:      search path=/glibc/build/tst-bz25263:/nix/store/6j2qcdi8axcl91v08blc3vjn39y117z3-shell/lib:/nix/store/q6kvdfhlp251n8rx183lg1n245kwg6m8-glibc-2.39-52/lib:/nix/store/fxh2cg0sfgygfya6qjr11gzj4sbjx0h9-gcc-13.2.0-lib/lib              (RUNPATH from file ./maincopy)
>      49073:       trying file=/glibc/build/tst-bz25263/lib1.so
>      49073:       trying file=/nix/store/6j2qcdi8axcl91v08blc3vjn39y117z3-shell/lib/lib1.so
>      49073:       trying file=/nix/store/q6kvdfhlp251n8rx183lg1n245kwg6m8-glibc-2.39-52/lib/lib1.so
>      49073:       trying file=/nix/store/fxh2cg0sfgygfya6qjr11gzj4sbjx0h9-gcc-13.2.0-lib/lib/lib1.so
>      49073:      search cache=/root/etc/ld.so.cache
>      49073:      search path=/root/lib          (system search path)
>      49073:       trying file=/root/lib/lib1.so
>      49073:
> ./maincopy: error while loading shared libraries: lib1.so: cannot open shared object file: No such file or directory
> 
> What commit and OS and arch are you on? (It's possible I'm confusing 
> myself by using NixOS, maybe.)
> 

So I don't have a strong preference, I started to work on a similar fix [1]
with my proposed approach.  It has the slight advantage that realpath will
allocate just the required size and work with path larger than PATH_MAX 
(which we try to impose on most interfaces, but I think this will be 
unlikely to be a problem), although it will require some more syscall to
canonicalize.

https://github.com/zatrazz/glibc/tree/azanella/bz25263-loader-realpath
Adhemerval Zanella March 3, 2025, 8:08 p.m. UTC | #4 
On 28/02/25 22:27, Adhemerval Zanella Netto wrote:
> 
> 
> On 28/02/25 16:02, Geoffrey Thomas wrote:
>> On Thu, Feb 27, 2025, at 3:26 PM, Adhemerval Zanella Netto wrote:
>>> Although this change solves the LD_TRACE_LOADED_OBJECTS=1 (ldd) issue, I
>>> think it still misses the underlying issue for the case user want to also 
>>> run the binary with a direct loader invocation (as Florian has hinted on 
>>> comment #4).
>>>
>>> For instance, with this testcase (similar to what electron is doing from
>>> the bug report):
>>>
>>> $ cat test/main.c
>>> int main (int argc, char *argv[]) { return 0; }
>>> $ cat test/lib1.c
>>> int foo (void) { return 42; }
>>> tst-bz25263 $ mkdir test && cd test
>>> test $ gcc -Wall -shared -Wl,-soname,lib1.so lib1.c -o lib1.so
>>> test $ gcc -no-pie -Wall main.c -o main -Wl,-rpath,\$ORIGIN -L`pwd` 
>>> -Wl,-no-as-needed -l1
>>> test $ gcc -no-pie -Wall main.c -o main-hcp 
>>> -Wl,-rpath,\$ORIGIN:$GLIBCBUILDDIR 
>>> -Wl,-dynamic-linker,$GLIBCBUILDDIR/elf/ld.so -L`pwd` -Wl,-no-as-needed 
>>> -l1
>>> main $ cd ..
>>> $ ln -s test/main main
>>> $ ln -s test/main-hcp main-hcp
>>>
>>> So 'main' is a binary without just one RUNPATH with $ORIGIN, while 'main-hcp'
>>> is binary that will use a built glibc (along with the RUNPATH).
>>>
>>> Checking the library path resolution for both cases:
>>>
>>> $ LD_DEBUG=libs ./main-hcp
>>>    3151277:     find library=lib1.so [0]; searching
>>>    3151277:      search 
>>> path=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v3:/home/azanella/Projects/glibc/build/
>>> x86_64-linux-gnu-work/tst-bz25263/test/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR/tst-bz25263/test:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR 
>>>          (RUNPATH from file ./main-hcp)
>>>    3151277:       trying 
>>> file=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v3/lib1.so
>>>    3151277:       trying 
>>> file=$GLIBCBUILDDIR/tst-bz25263/test/glibc-hwcaps/x86-64-v2/lib1.so
>>>    3151277:       trying file=$GLIBCBUILDDIR/tst-bz25263/test/lib1.so
>>>
>>> LD_DEBUG=libs $GLIBCBUILDDIR/elf/ld.so --library-path $GLIBCBUILDDIR 
>>> ./main
>>>    3151286:	find library=lib1.so [0]; searching
>>>    3151286:	 search 
>>> path=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR		(--library-path)
>>>    3151286:	  trying file=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v3/lib1.so
>>>    3151286:	  trying file=$GLIBCBUILDDIR/glibc-hwcaps/x86-64-v2/lib1.so
>>>    3151286:	  trying file=$GLIBCBUILDDIR/lib1.so
>>>    3151286:	 search 
>>> path=$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v3:$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v2:$GLIBCBUILDDIR/tst-bz25263/.		(RUNPATH 
>>> from file ./main)
>>>    3151286:	  trying 
>>> file=$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v3/lib1.so
>>>    3151286:	  trying 
>>> file=$GLIBCBUILDDIR/tst-bz25263/./glibc-hwcaps/x86-64-v2/lib1.so
>>>    3151286:	  trying file=$GLIBCBUILDDIR/tst-bz25263/./lib1.so
>>>    3151286:	 search cache=/etc/ld.so.cache
>>>    3151286:	 search 
>>> path=/lib64/glibc-hwcaps/x86-64-v3:/lib64/glibc-hwcaps/x86-64-v2:/lib64:/usr/lib64/glibc-hwcaps/x86-64-v3:/usr/lib64/glibc-hwcaps/x86-64-v2:/usr/lib64		(system 
>>> search path)
>>>    3151286:	  trying file=/lib64/glibc-hwcaps/x86-64-v3/lib1.so
>>>    3151286:	  trying file=/lib64/glibc-hwcaps/x86-64-v2/lib1.so
>>>    3151286:	  trying file=/lib64/lib1.so
>>>    3151286:	  trying file=/usr/lib64/glibc-hwcaps/x86-64-v3/lib1.so
>>>    3151286:	  trying file=/usr/lib64/glibc-hwcaps/x86-64-v2/lib1.so
>>>    3151286:	  trying file=/usr/lib64/lib1.so
>>>
>>> You can see that if the binary is not issued directly by the loader 
>>> (main-hcp),
>>> it will have the full realpath on the search path 
>>> (GLIBCBUILDDIR/tst-bz25263/test);
>>> different that binary issued by the loader which is has the current 
>>> directory
>>> (GLIBCBUILDDIR/tst-bz25263).
>>
>> This is with my patch? Directly on top of glibc-2.41 (commit db71c940
>> from github.com/geofft/glibc), with a checkout at /glibc and so
>> GLIBCBUILDDIR=/glibc/build, I get
> 
> Yes, this your patch on top of 9e51ae3cd0c7f65bdeba93b7f1d780cdb21fc269
> 
>>
>> [root@nixos:/glibc/build/tst-bz25263]# ls -l
>> total 0
>> lrwxr-xr-x 1 root root   9 Feb 28 13:37 main -> test/main
>> lrwxr-xr-x 1 root root  13 Feb 28 13:37 main-hcp -> test/main-hcp
>> drwxr-xr-x 7 root root 224 Feb 28 13:37 test
>> [root@nixos:/glibc/build/tst-bz25263]# readelf -d main | grep NEEDED
>>  0x0000000000000001 (NEEDED)             Shared library: [lib1.so]
>>  0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
>> [root@nixos:/glibc/build/tst-bz25263]# LD_DEBUG=libs /glibc/build/elf/ld.so --library-path /glibc/build ./main
>>      49044:     find library=lib1.so [0]; searching
>>      49044:      search path=/glibc/build               (--library-path)
>>      49044:       trying file=/glibc/build/lib1.so
>>      49044:      search path=/glibc/build/tst-bz25263/test:/nix/store/6j2qcdi8axcl91v08blc3vjn39y117z3-shell/lib:/nix/store/q6kvdfhlp251n8rx183lg1n245kwg6m8-glibc-2.39-52/lib:/nix/store/fxh2cg0sfgygfya6qjr11gzj4sbjx0h9-gcc-13.2.0-lib/lib         (RUNPATH from file ./main)
>>      49044:       trying file=/glibc/build/tst-bz25263/test/lib1.so
>>      49044:
>>      49044:     find library=libc.so.6 [0]; searching
>>      49044:      search path=/glibc/build               (--library-path)
>>      49044:       trying file=/glibc/build/libc.so.6
>> [...]
>>
>> which, if I'm following correctly, is the fixed behavior.
> 
> Sigh, for some reason I did not use git-pw on your patch and applied
> directly from mailbox it was malformated with the wrong path for the
> procfs. With 'git-pw patch apply 106751' it now seems to work.
> 
> You might want to use this instead (we have a helper to access the
> procfs).
> 
>   struct fd_to_filename fdfilename;
>   char *canonical = (char *) malloc (PATH_MAX + 1);
>   char *path = __fd_to_filename (fd, &fdfilename);
>   int size = INTERNAL_SYSCALL_CALL (readlinkat, AT_FDCWD, path,
>                                     canonical, PATH_MAX);
> 
> It does seems to work.

I like your approach better in fact, I was trying to make it generic
with realpath and it pulls too many dependencies on Hurd that I would
need to make it Linux specific anyway.

However I think we should make the _dl_canonicalize simpler and not
mess with the input argument since all the logic in the caller 
(_dl_map_object_from_fd).  Also, limit the maximum path to PATH_MAX
(and not PATH_MAX + 1) similar to _dl_get_origin and optimize the
memory allocation a bit with __strdup:

char *
_dl_canonicalize (int fd)
{
  struct fd_to_filename fdfilename;
  char canonical[PATH_MAX];
  char *path = __fd_to_filename (fd, &fdfilename);
  int size = INTERNAL_SYSCALL_CALL (readlinkat, AT_FDCWD, path,
                                    canonical, PATH_MAX - 1);
  if (size >= 0)
    {
      canonical[size] = '\0';
      return __strdup (canonical);
    }
  return NULL;
}

I added this suggestion along with a testcase [1], what do you
think?

[1] https://sourceware.org/git/?p=glibc.git;a=commit;h=372c632ce7c78471dbda69ca33625d1ecb6fb2f7

[1] https://sourceware.org/git?p=glibc.git;a=commit;h=372c632ce7c78471dbda69ca33625d1ecb6fb2f7
Geoffrey Thomas March 10, 2025, 6:19 p.m. UTC | #5 
On Mon, Mar 3, 2025, at 3:08 PM, Adhemerval Zanella Netto wrote:
> On 28/02/25 22:27, Adhemerval Zanella Netto wrote:
>> Sigh, for some reason I did not use git-pw on your patch and applied
>> directly from mailbox it was malformated with the wrong path for the
>> procfs. With 'git-pw patch apply 106751' it now seems to work.

Oh no, is that something I misconfigured/broke on my end? I just used
git send-email...

> I like your approach better in fact, I was trying to make it generic
> with realpath and it pulls too many dependencies on Hurd that I would
> need to make it Linux specific anyway.

Note that the behavior is different on Hurd so I think we should indeed
handle this as a Linux-specific thing for now.

The generic elf/dl-origin.c looks only at $LD_ORIGIN_PATH. This is set
by the Hurd exec server [1] to the absolute path passed to it. The
Linux-specific code in sysdeps/unix/sysv/linux/dl-origin.c is the one
that looks at /proc/self/exe.

[1] https://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/exec/exec.c?h=v0.9.git20250304#n948

Hurd does not canonicalize the path in $LD_ORIGIN_PATH at all:

geofft@hurd:~/one$ cat main.c
int main(void) {}
geofft@hurd:~/one$ cc -fPIC -shared -o libfoo.so -lc
geofft@hurd:~/one$ cc -o main main.c -Wl,--no-as-needed -lfoo -Wl,--as-needed -L. -Wl,-rpath,\$ORIGIN
geofft@hurd:~/one$ cd ../two
geofft@hurd:~/two$ ln -s ../one/main
geofft@hurd:~/two$ ./main
./main: error while loading shared libraries: libfoo.so: cannot open
shared object file: No such file or directory

So, for consistency, we should not canonicalize the path either. On the
other hand, as a consequence of $LD_ORIGIN_PATH being set (I think?), we
have a more serious misbehavior with ldd on Hurd:

geofft@hurd:~/two$ cd ../one
geofft@hurd:~/one$ ./main
geofft@hurd:~/one$ ldd ./main | grep libfoo
        libfoo.so => not found
geofft@hurd:~/one$ LD_TRACE_LOADED_OBJECTS=1 ./main | grep libfoo
        libfoo.so => /home/geofft/one/./libfoo.so (0x0103f000)
geofft@hurd:~/one$ /lib/ld.so ./main
./main: error while loading shared libraries: libfoo.so: cannot open
shared object file: No such file or directory
geofft@hurd:~/one$ env | grep LD_ORIGIN_PATH
LD_ORIGIN_PATH=/usr/bin
geofft@hurd:~/one$ /lib/ld.so /usr/bin/env | grep LD_ORIGIN_PATH
LD_ORIGIN_PATH=/lib

Probably the correct thing to do is to a) have the argument to ld.so
take precedence over $LD_ORIGIN_PATH and b) canonicalize (ourselves, via
realpath) $LD_ORIGIN_PATH in all circumstances. But that seems like a
bigger change. (On the other hand, now that I've installed a Hurd
VM, maybe you could get me to do it....)

This seems to have been reported previously at
https://sourceware.org/bugzilla/show_bug.cgi?id=24260

> However I think we should make the _dl_canonicalize simpler and not
> mess with the input argument since all the logic in the caller 
> (_dl_map_object_from_fd).  Also, limit the maximum path to PATH_MAX
> (and not PATH_MAX + 1) similar to _dl_get_origin and optimize the
> memory allocation a bit with __strdup:
>
> char *
> _dl_canonicalize (int fd)
> {
>   struct fd_to_filename fdfilename;
>   char canonical[PATH_MAX];
>   char *path = __fd_to_filename (fd, &fdfilename);
>   int size = INTERNAL_SYSCALL_CALL (readlinkat, AT_FDCWD, path,
>                                     canonical, PATH_MAX - 1);
>   if (size >= 0)
>     {
>       canonical[size] = '\0';
>       return __strdup (canonical);
>     }
>   return NULL;
> }
>
> I added this suggestion along with a testcase [1], what do you
> think?
>
> [1] 
> https://sourceware.org/git/?p=glibc.git;a=commit;h=372c632ce7c78471dbda69ca33625d1ecb6fb2f7

On rereading the readlink man page, I think we should fail to
canonicalize if the return value from readlink is equal to the passed
buffer size, because then we don't know if the path was truncated or
not, and I am worried about a case where the path is truncated and an
attacker controls the truncated path name. Specifically, I think just
changing to

  if (size >= 0 && size < PATH_MAX - 1)

would do the trick. (I do see a reference to PATH_MAX in the kernel
implementation of readlink on /proc/$pid/fd, but I'm not sure if that's
an ABI promise or if the kernel can return longer results in the future.)

I like the change to return a pointer and not mess with calling free()
in the subroutine, and thanks for mentioning __fd_to_filename. The test
looks good to me other than that I think the commented non-ldd test
ought to be uncommented - I think both cases are expected to work (on
Linux) and the test should fail if either case fails. Thanks for the
reworking and the review! Feel free to add my signoff.
Adhemerval Zanella March 11, 2025, 1:02 p.m. UTC | #6 
On 10/03/25 15:19, Geoffrey Thomas wrote:
> On Mon, Mar 3, 2025, at 3:08 PM, Adhemerval Zanella Netto wrote:
>> On 28/02/25 22:27, Adhemerval Zanella Netto wrote:
>>> Sigh, for some reason I did not use git-pw on your patch and applied
>>> directly from mailbox it was malformated with the wrong path for the
>>> procfs. With 'git-pw patch apply 106751' it now seems to work.
> 
> Oh no, is that something I misconfigured/broke on my end? I just used
> git send-email...

No, it is totally on my side.

> 
>> I like your approach better in fact, I was trying to make it generic
>> with realpath and it pulls too many dependencies on Hurd that I would
>> need to make it Linux specific anyway.
> 
> Note that the behavior is different on Hurd so I think we should indeed
> handle this as a Linux-specific thing for now.
> 
> The generic elf/dl-origin.c looks only at $LD_ORIGIN_PATH. This is set
> by the Hurd exec server [1] to the absolute path passed to it. The
> Linux-specific code in sysdeps/unix/sysv/linux/dl-origin.c is the one
> that looks at /proc/self/exe.
> 
> [1] https://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/exec/exec.c?h=v0.9.git20250304#n948
> 
> Hurd does not canonicalize the path in $LD_ORIGIN_PATH at all:
> 
> geofft@hurd:~/one$ cat main.c
> int main(void) {}
> geofft@hurd:~/one$ cc -fPIC -shared -o libfoo.so -lc
> geofft@hurd:~/one$ cc -o main main.c -Wl,--no-as-needed -lfoo -Wl,--as-needed -L. -Wl,-rpath,\$ORIGIN
> geofft@hurd:~/one$ cd ../two
> geofft@hurd:~/two$ ln -s ../one/main
> geofft@hurd:~/two$ ./main
> ./main: error while loading shared libraries: libfoo.so: cannot open
> shared object file: No such file or directory
> 
> So, for consistency, we should not canonicalize the path either. On the
> other hand, as a consequence of $LD_ORIGIN_PATH being set (I think?), we
> have a more serious misbehavior with ldd on Hurd:
> 
> geofft@hurd:~/two$ cd ../one
> geofft@hurd:~/one$ ./main
> geofft@hurd:~/one$ ldd ./main | grep libfoo
>         libfoo.so => not found
> geofft@hurd:~/one$ LD_TRACE_LOADED_OBJECTS=1 ./main | grep libfoo
>         libfoo.so => /home/geofft/one/./libfoo.so (0x0103f000)
> geofft@hurd:~/one$ /lib/ld.so ./main
> ./main: error while loading shared libraries: libfoo.so: cannot open
> shared object file: No such file or directory
> geofft@hurd:~/one$ env | grep LD_ORIGIN_PATH
> LD_ORIGIN_PATH=/usr/bin
> geofft@hurd:~/one$ /lib/ld.so /usr/bin/env | grep LD_ORIGIN_PATH
> LD_ORIGIN_PATH=/lib
> 
> Probably the correct thing to do is to a) have the argument to ld.so
> take precedence over $LD_ORIGIN_PATH and b) canonicalize (ourselves, via
> realpath) $LD_ORIGIN_PATH in all circumstances. But that seems like a
> bigger change. (On the other hand, now that I've installed a Hurd
> VM, maybe you could get me to do it....)
> 
> This seems to have been reported previously at
> https://sourceware.org/bugzilla/show_bug.cgi?id=24260

Hurd setting LD_ORIGIN_PATH on the exec server seems like a hack solution
imho, but maybe this the cleanest way on Hurd to have this information
passed to the loader.  I though about check if we can remove this from
Linux, but it seems that there is the corner case where if procfs is
not accessible, LD_ORIGIN_PATH is the only way to get a binary with
$ORIGIN to work correctly in all cases.

And I think your suggestion to fix on Hurd seems correct, although to
pull realpath on the loader it would require a lot of changes.

> 
>> However I think we should make the _dl_canonicalize simpler and not
>> mess with the input argument since all the logic in the caller 
>> (_dl_map_object_from_fd).  Also, limit the maximum path to PATH_MAX
>> (and not PATH_MAX + 1) similar to _dl_get_origin and optimize the
>> memory allocation a bit with __strdup:
>>
>> char *
>> _dl_canonicalize (int fd)
>> {
>>   struct fd_to_filename fdfilename;
>>   char canonical[PATH_MAX];
>>   char *path = __fd_to_filename (fd, &fdfilename);
>>   int size = INTERNAL_SYSCALL_CALL (readlinkat, AT_FDCWD, path,
>>                                     canonical, PATH_MAX - 1);
>>   if (size >= 0)
>>     {
>>       canonical[size] = '\0';
>>       return __strdup (canonical);
>>     }
>>   return NULL;
>> }
>>
>> I added this suggestion along with a testcase [1], what do you
>> think?
>>
>> [1] 
>> https://sourceware.org/git/?p=glibc.git;a=commit;h=372c632ce7c78471dbda69ca33625d1ecb6fb2f7
> 
> On rereading the readlink man page, I think we should fail to
> canonicalize if the return value from readlink is equal to the passed
> buffer size, because then we don't know if the path was truncated or
> not, and I am worried about a case where the path is truncated and an
> attacker controls the truncated path name. Specifically, I think just
> changing to
> 
>   if (size >= 0 && size < PATH_MAX - 1)
> 
> would do the trick. (I do see a reference to PATH_MAX in the kernel
> implementation of readlink on /proc/$pid/fd, but I'm not sure if that's
> an ABI promise or if the kernel can return longer results in the future.)

In fact I had something similar while I was working (not sure why I
removed it) and it does make sense to check for truncation.  

And for PATH_MAX on readlink on /proc/$pid/fd, I think it is because it 
would depend of the underlying filesystem support where the binary is
being placed.  So I think PATH_MAX is the minimum denominator here and 
it should be safe to assume that kernel always support it.

> 
> I like the change to return a pointer and not mess with calling free()
> in the subroutine, and thanks for mentioning __fd_to_filename. The test
> looks good to me other than that I think the commented non-ldd test
> ought to be uncommented - I think both cases are expected to work (on
> Linux) and the test should fail if either case fails. Thanks for the
> reworking and the review! Feel free to add my signoff.
> 

Oops, I will fix it.  I will send a new version with these fixed and
if you can add your reviewed-by I will install it.

Thanks for working on this.
diff mbox series

Patch

diff --git a/elf/dl-load.c b/elf/dl-load.c
index f905578a65..2ef585bc5c 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -965,6 +965,7 @@  _dl_map_object_from_fd (const char *name, const char *origname, int fd,
     {
       assert (nsid == LM_ID_BASE);
       memset (&id, 0, sizeof (id));
+      _dl_canonicalize (&realname, fd);
     }
   else
     {
diff --git a/elf/dl-origin.c b/elf/dl-origin.c
index 9f6b921b01..d942f3f0bb 100644
--- a/elf/dl-origin.c
+++ b/elf/dl-origin.c
@@ -47,3 +47,8 @@  _dl_get_origin (void)
 
   return result;
 }
+
+void
+_dl_canonicalize (char **filename, int fd)
+{
+}
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index e871f27ff2..36b472e19c 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -1198,6 +1198,11 @@  extern struct link_map * _dl_get_dl_main_map (void) attribute_hidden;
 /* Find origin of the executable.  */
 extern const char *_dl_get_origin (void) attribute_hidden;
 
+/* Canonicalize the path to an open file.  FILENAME is a pointer to a
+   string allocated with malloc; it may be freed and replaced with
+   another string allocated with malloc. */
+extern void _dl_canonicalize (char **filename, int fd) attribute_hidden;
+
 /* Count DSTs.  */
 extern size_t _dl_dst_count (const char *name) attribute_hidden;
 
diff --git a/sysdeps/unix/sysv/linux/dl-origin.c b/sysdeps/unix/sysv/linux/dl-origin.c
index decdd8ae9e..b12eff4315 100644
--- a/sysdeps/unix/sysv/linux/dl-origin.c
+++ b/sysdeps/unix/sysv/linux/dl-origin.c
@@ -72,3 +72,30 @@  _dl_get_origin (void)
 
   return result;
 }
+
+/* On Linux, readlink on the magic symlinks in /proc/self/fd also has
+   the same behavior of returning the canonical path from the dcache.
+   If it does not work, we do not bother to canonicalize. */
+
+void
+_dl_canonicalize (char **filename, int fd)
+{
+  char *canonical = (char *) malloc (PATH_MAX + 1);
+  char buf[25];
+  buf[24] = '\0';
+  char *path = _itoa (fd, buf + 24, 10, 0);
+  path = memcpy (path - 14, "/proc/self/fd/", 14);
+
+  int size = INTERNAL_SYSCALL_CALL (readlinkat, AT_FDCWD, path,
+				    canonical, PATH_MAX );
+  if (size >= 0)
+    {
+      free (*filename);
+      canonical[size] = '\0';
+      *filename = canonical;
+    }
+  else
+    {
+      free (canonical);
+    }
+}