elf: Restore lconfig libc6 implicit soname logic [BZ #30125]

While cleaning up old libc version support, the deprecated libc4 code was
accidentally kept in `implicit_soname`, instead of the libc6 code.

This causes additional symlinks to be created by `ldconfig` for libraries
without a soname, e.g. a library `libsomething.123.456.789` without a soname
will create a `libsomething.123` -> `libsomething.123.456.789` symlink.

As the libc6 version of the `implicit_soname` code is just a trivial xstrdup,
just inline it and remove `implicit_soname` altogether.

Some further simplification looks possible (e.g. the call to `create_links`
looks like a no-op if `soname == NULL`, other than the verbose printfs), but
logic is kept as-is for now.

Fixes: BZ #30125
Fixes: 8ee878592c4a ("Assume only FLAG_ELF_LIBC6 suport")
Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com>
---
 elf/Makefile                  |  7 ++++++
 elf/ldconfig.c                |  4 +--
 elf/readlib.c                 | 19 ---------------
 elf/tst-ldconfig-no-soname.sh | 46 +++++++++++++++++++++++++++++++++++
 sysdeps/generic/ldconfig.h    |  2 --
 5 files changed, 55 insertions(+), 23 deletions(-)
 create mode 100644 elf/tst-ldconfig-no-soname.sh

Sorry, typo due to last-minute edit: s/lconfig/ldconfig/ in the subject

On 14/02/23 22:41, Joan Bruguera wrote:
> While cleaning up old libc version support, the deprecated libc4 code was
> accidentally kept in `implicit_soname`, instead of the libc6 code.
> 
> This causes additional symlinks to be created by `ldconfig` for libraries
> without a soname, e.g. a library `libsomething.123.456.789` without a soname
> will create a `libsomething.123` -> `libsomething.123.456.789` symlink.
> 
> As the libc6 version of the `implicit_soname` code is just a trivial xstrdup,
> just inline it and remove `implicit_soname` altogether.
> 
> Some further simplification looks possible (e.g. the call to `create_links`
> looks like a no-op if `soname == NULL`, other than the verbose printfs), but
> logic is kept as-is for now.
> 
> Fixes: BZ #30125
> Fixes: 8ee878592c4a ("Assume only FLAG_ELF_LIBC6 suport")

Patch looks ok in general, some comments on the test.  You used an old revision
and then patch does not apply cleanly[1], could you rebase the patch and send 
a v2?

[1] https://patchwork.sourceware.org/project/glibc/patch/20230215014124.4144451-1-joanbrugueram@gmail.com/

> Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com>

We do not signed-off-by on glibc, so no need for it.

> ---
>  elf/Makefile                  |  7 ++++++
>  elf/ldconfig.c                |  4 +--
>  elf/readlib.c                 | 19 ---------------
>  elf/tst-ldconfig-no-soname.sh | 46 +++++++++++++++++++++++++++++++++++
>  sysdeps/generic/ldconfig.h    |  2 --
>  5 files changed, 55 insertions(+), 23 deletions(-)
>  create mode 100644 elf/tst-ldconfig-no-soname.sh
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index b509b3eada..d5f541fd84 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -626,6 +626,7 @@ ifeq ($(run-built-tests),yes)
>  tests-special += \
>    $(objpfx)noload-mem.out \
>    $(objpfx)tst-ldconfig-X.out \
> +  $(objpfx)tst-ldconfig-no-soname.out \
>    $(objpfx)tst-leaks1-mem.out \
>    $(objpfx)tst-rtld-help.out \
>    # tests-special
> @@ -2396,6 +2397,12 @@ $(objpfx)tst-ldconfig-X.out : tst-ldconfig-X.sh $(objpfx)ldconfig
>  		 '$(run-program-env)' > $@; \
>  	$(evaluate-test)
>  
> +$(objpfx)tst-ldconfig-no-soname.out : tst-ldconfig-no-soname.sh \
> +  $(objpfx)ldconfig
> +	$(SHELL) $< '$(common-objpfx)' '$(test-wrapper-env)' \
> +		 '$(run-program-env)' > $@; \
> +	$(evaluate-test)
> +
>  # Test static linking of all the libraries we can possibly link
>  # together.  Note that in some configurations this may be less than the
>  # complete list of libraries we build but we try to maxmimize this list.
> diff --git a/elf/ldconfig.c b/elf/ldconfig.c
> index 3f1b30c570..2fc45ad825 100644
> --- a/elf/ldconfig.c
> +++ b/elf/ldconfig.c
> @@ -616,7 +616,7 @@ manual_link (char *library)
>        goto out;
>      }
>    if (soname == NULL)
> -    soname = implicit_soname (libname, flag);
> +    soname = xstrdup (libname);
>    create_links (real_path, path, libname, soname);
>    free (soname);
>  out:
> @@ -849,7 +849,7 @@ search_dir (const struct dir_entry *entry)
>  	}
>  
>        if (soname == NULL)
> -	soname = implicit_soname (direntry->d_name, flag);
> +	soname = xstrdup (direntry->d_name);
>  
>        /* A link may just point to itself.  */
>        if (is_link)
> diff --git a/elf/readlib.c b/elf/readlib.c
> index c5c3591eef..bc13d9acc6 100644
> --- a/elf/readlib.c
> +++ b/elf/readlib.c
> @@ -166,24 +166,5 @@ process_file (const char *real_file_name, const char *file_name,
>    return ret;
>  }
>  
> -/* Returns made up soname if lib doesn't have explicit DT_SONAME.  */
> -
> -char *
> -implicit_soname (const char *lib, int flag)
> -{
> -  char *soname = xstrdup (lib);
> -
> -  /* Aout files don't have a soname, just return the name
> -     including the major number.  */
> -  char *major = strstr (soname, ".so.");
> -  if (major)
> -    {
> -      char *dot = strstr (major + 4, ".");
> -      if (dot)
> -	*dot = '\0';
> -    }
> -  return soname;
> -}
> -
>  /* Get architecture specific version of process_elf_file.  */
>  #include <readelflib.c>
> diff --git a/elf/tst-ldconfig-no-soname.sh b/elf/tst-ldconfig-no-soname.sh
> new file mode 100644
> index 0000000000..c60913d75a
> --- /dev/null
> +++ b/elf/tst-ldconfig-no-soname.sh
> @@ -0,0 +1,46 @@
> +#!/bin/sh
> +# Test that ldconfig not create symbolic links for libraries without a soname.
> +# Copyright (C) 2023 Free Software Foundation, Inc.
> +# This file is part of the GNU C Library.
> +
> +# The GNU C Library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2.1 of the License, or (at your option) any later version.
> +
> +# The GNU C Library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# Lesser General Public License for more details.
> +
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with the GNU C Library; if not, see
> +# <https://www.gnu.org/licenses/>.
> +
> +set -ex
> +
> +common_objpfx=$1
> +test_wrapper_env=$2
> +run_program_env=$3
> +
> +testroot="${common_objpfx}elf/bug30125-test-directory"
> +cleanup () {
> +    rm -rf "$testroot"
> +}
> +trap cleanup 0
> +
> +rm -rf "$testroot"
> +mkdir -p $testroot/lib $testroot/etc
> +
> +gcc /dev/null -o $testroot/lib/libsomething.so.1.2.3 -shared

We can not assume gcc will be presented in PATH, you need to use the CC used
on build.  So pass the $(CC) on tst-ldconfig-no-soname.out rule and use it on
the script.

> +
> +${test_wrapper_env} \
> +${run_program_env} \
> +${common_objpfx}elf/ldconfig -f /dev/null \
> +  -C $testroot/etc/ld.so.cache \
> +  $testroot/lib
> +
> +if [ -n "$(find $testroot/lib -type l)" ]; then
> +    echo "error: $0 - symlink found"
> +    exit 1
> +fi
> diff --git a/sysdeps/generic/ldconfig.h b/sysdeps/generic/ldconfig.h
> index 0e1a9a9515..e9e9e19d0f 100644
> --- a/sysdeps/generic/ldconfig.h
> +++ b/sysdeps/generic/ldconfig.h
> @@ -90,8 +90,6 @@ extern int process_file (const char *real_file_name, const char *file_name,
>  			 const char *lib, int *flag, unsigned int *isa_level,
>  			 char **soname, int is_link, struct stat *stat_buf);
>  
> -extern char *implicit_soname (const char *lib, int flag);
> -
>  /* Declared in readelflib.c.  */
>  extern int process_elf_file (const char *file_name, const char *lib,
>  			     int *flag, unsigned int *isa_level, char **soname,

Thank you for the review, I have submitted a v2 adressing your points.

> On 15 Feb 2023, at 13:19, Adhemerval Zanella Netto via Libc-alpha <libc-alpha@sourceware.org> wrote:
> 
> 
> 
> On 14/02/23 22:41, Joan Bruguera wrote:
>> While cleaning up old libc version support, the deprecated libc4 code was
>> accidentally kept in `implicit_soname`, instead of the libc6 code.
>> 
>> This causes additional symlinks to be created by `ldconfig` for libraries
>> without a soname, e.g. a library `libsomething.123.456.789` without a soname
>> will create a `libsomething.123` -> `libsomething.123.456.789` symlink.
>> 
>> As the libc6 version of the `implicit_soname` code is just a trivial xstrdup,
>> just inline it and remove `implicit_soname` altogether.
>> 
>> Some further simplification looks possible (e.g. the call to `create_links`
>> looks like a no-op if `soname == NULL`, other than the verbose printfs), but
>> logic is kept as-is for now.
>> 
>> Fixes: BZ #30125
>> Fixes: 8ee878592c4a ("Assume only FLAG_ELF_LIBC6 suport")
> 
> Patch looks ok in general, some comments on the test.  You used an old revision
> and then patch does not apply cleanly[1], could you rebase the patch and send
> a v2?
> 
> [1] https://patchwork.sourceware.org/project/glibc/patch/20230215014124.4144451-1-joanbrugueram@gmail.com/
> 
>> Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com>
> 
> We do not signed-off-by on glibc, so no need for it.
> 

https://sourceware.org/glibc/wiki/Contribution%20checklist#Developer_Certificate_of_Origin says we do?

>> [snip]

>> +gcc /dev/null -o $testroot/lib/libsomething.so.1.2.3 -shared
> 
> We can not assume gcc will be presented in PATH, you need to use the CC used
> on build.  So pass the $(CC) on tst-ldconfig-no-soname.out rule and use it on
> the script.

(Indeed, GCC may not be the used compiler either, or specifically 'gcc' but some
prefixed or versioned gcc).

>> 
>>> Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com>
>> 
>> We do not signed-off-by on glibc, so no need for it.
>> 
> 
> https://sourceware.org/glibc/wiki/Contribution%20checklist#Developer_Certificate_of_Origin says we do?

Hmmm, indeed, after re-re-reviewing this document, I understand (hopefully
correctly) that I indeed *must* include the Signed-off-by header, as I am not
covered by a copyright assignment to the FSF.

I have submitted a v3 adding the Signed-off-by as well as a
"Copyright The GNU Toolchain Authors." header to the new test,
additionally to a "Copyright (C) 2000-2023 Free Software Foundation, Inc."
(as the test structure is based on tst-ldconfig-X.sh.)
Apologies if *not* adding the "Copyright The GNU Toolchain Authors." header in
the previous mail made the reviewer think I was covered by a FSF assignment.

Hope I've got the copyright legalese right now.

* Joan Bruguera via Libc-alpha:

>>> 
>>>> Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com>
>>> 
>>> We do not signed-off-by on glibc, so no need for it.
>>> 
>> 
>> https://sourceware.org/glibc/wiki/Contribution%20checklist#Developer_Certificate_of_Origin says we do?
>
> Hmmm, indeed, after re-re-reviewing this document, I understand (hopefully
> correctly) that I indeed *must* include the Signed-off-by header, as I am not
> covered by a copyright assignment to the FSF.

Yes, that's true.

> I have submitted a v3 adding the Signed-off-by as well as a
> "Copyright The GNU Toolchain Authors." header to the new test,
> additionally to a "Copyright (C) 2000-2023 Free Software Foundation, Inc."
> (as the test structure is based on tst-ldconfig-X.sh.)
> Apologies if *not* adding the "Copyright The GNU Toolchain Authors." header in
> the previous mail made the reviewer think I was covered by a FSF assignment.

That's what I've been doing in similar cases.  Thanks for the attention
to detail.

Florian