ldconfig: Do not remove stale symbolic links with -X [BZ #19610]

On 02/12/2016 07:34 PM, Carlos O'Donell wrote:

> At the design level:
> - Comment for opt_link needs to change. Expand the comment on
>   line 103 in elf/ldconfig.c to describe in more detail that
>   symlinks are not created, and that neither are they removed
>   if they exist and are stale.

Done.

> - Use mktemp to create a temporary directory that exists instead
>   of using /opt which might be problematic.

Thanks.  I'm using the existing directory tree instead.  I've added
comments to the test explaining the relative vs absolute difference.

New version attached.

Florian

On 02/19/2016 11:50 AM, Florian Weimer wrote:
> On 02/12/2016 07:34 PM, Carlos O'Donell wrote:
> 
>> > At the design level:
>> > - Comment for opt_link needs to change. Expand the comment on
>> >   line 103 in elf/ldconfig.c to describe in more detail that
>> >   symlinks are not created, and that neither are they removed
>> >   if they exist and are stale.
> Done.
> 
>> > - Use mktemp to create a temporary directory that exists instead
>> >   of using /opt which might be problematic.
> Thanks.  I'm using the existing directory tree instead.  I've added
> comments to the test explaining the relative vs absolute difference.
> 
> New version attached.
> 
> Florian
> 
> 
> 0001-ldconfig-Do-not-remove-stale-symbolic-links-with-X-B.patch
> 
> 
> 2016-02-19  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #19610]
> 	* elf/ldconfig.c (opt_link): Update comment.
> 	(options): Update help string for option -X.
> 	(search_dir): Unlink stale symbolic link only if updating symbolic
> 	links.
> 	* elf/tst-ldconfig-X.sh: New file.
> 	* elf/Makefile (tests-special): Add tst-ldconfig-X.out.
> 	(tst-ldconfig-X.out): New rule to run tst-ldconfig-X.sh.

Looks perfect to me. Thanks.

Please commit.

> diff --git a/elf/Makefile b/elf/Makefile
> index 63a5355..ae99f65 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -163,7 +163,8 @@ endif
>  endif
>  ifeq ($(run-built-tests),yes)
>  tests-special += $(objpfx)tst-leaks1-mem.out \
> -		 $(objpfx)tst-leaks1-static-mem.out $(objpfx)noload-mem.out
> +		 $(objpfx)tst-leaks1-static-mem.out $(objpfx)noload-mem.out \
> +		 $(objpfx)tst-ldconfig-X.out

OK.

>  endif
>  tlsmod17a-suffixes = 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
>  tlsmod18a-suffixes = 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
> @@ -1252,3 +1253,7 @@ $(objpfx)tst-prelink-cmp.out: tst-prelink.exp \
>  			      $(objpfx)tst-prelink-conflict.out
>  	cmp $^ > $@; \
>  	$(evaluate-test)
> +
> +$(objpfx)tst-ldconfig-X.out : tst-ldconfig-X.sh $(objpfx)ldconfig
> +	$(SHELL) $< '$(common-objpfx)' '$(test-wrapper)' '$(test-wrapper-env)' > $@; \
> +	$(evaluate-test)

OK.

> diff --git a/elf/ldconfig.c b/elf/ldconfig.c
> index 9c6f2ba..467ca82 100644
> --- a/elf/ldconfig.c
> +++ b/elf/ldconfig.c
> @@ -100,7 +100,8 @@ int opt_format = 1;
>  /* Build cache.  */
>  static int opt_build_cache = 1;
>  
> -/* Generate links.  */
> +/* Enable symbolic link processing.  If set, create or update symbolic
> +   links, and remove stale symbolic links.  */
>  static int opt_link = 1;

OK. Perfect.

>  
>  /* Only process directories specified on the command line.  */
> @@ -141,7 +142,7 @@ static const struct argp_option options[] =
>    { "print-cache", 'p', NULL, 0, N_("Print cache"), 0},
>    { "verbose", 'v', NULL, 0, N_("Generate verbose messages"), 0},
>    { NULL, 'N', NULL, 0, N_("Don't build cache"), 0},
> -  { NULL, 'X', NULL, 0, N_("Don't generate links"), 0},
> +  { NULL, 'X', NULL, 0, N_("Don't update symbolic links"), 0},

OK. Perfect.

>    { NULL, 'r', N_("ROOT"), 0, N_("Change to and use ROOT as root directory"), 0},
>    { NULL, 'C', N_("CACHE"), 0, N_("Use CACHE as cache file"), 0},
>    { NULL, 'f', N_("CONF"), 0, N_("Use CONF as configuration file"), 0},
> @@ -800,7 +801,7 @@ search_dir (const struct dir_entry *entry)
>  		error (0, errno, _("Cannot stat %s"), file_name);
>  
>  	      /* Remove stale symlinks.  */
> -	      if (strstr (direntry->d_name, ".so."))
> +	      if (opt_link && strstr (direntry->d_name, ".so."))

OK.

>  		unlink (real_file_name);
>  	      continue;
>  	    }
> diff --git a/elf/tst-ldconfig-X.sh b/elf/tst-ldconfig-X.sh
> new file mode 100644
> index 0000000..07fd7d2
> --- /dev/null
> +++ b/elf/tst-ldconfig-X.sh
> @@ -0,0 +1,63 @@
> +#!/bin/sh
> +# Test that ldconfig -X does not remove stale symbolic links.
> +# Copyright (C) 2000-2016 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
> +# <http://www.gnu.org/licenses/>.
> +
> +set -ex
> +
> +common_objpfx=$1
> +test_wrapper_env=$2
> +run_program_env=$3
> +
> +testroot="${common_objpfx}elf/bug19610-test-directory"
> +cleanup () {
> +    rm -rf "$testroot"
> +}
> +trap cleanup 0
> +
> +rm -rf "$testroot"
> +mkdir -p $testroot/lib $testroot/etc
> +
> +# Relative symbolic link target.
> +ln -s libdoesnotexist.so.1.1 $testroot/lib/libdoesnotexist.so.1
> +
> +# Absolute symbolic link target.
> +ln -s $testroot/opt/sw/lib/libdoesnotexist2.so.1.1 $testroot/lib/
> +
> +errors=0
> +check_files () {
> +    for name in libdoesnotexist.so.1 libdoesnotexist2.so.1.1 ; do
> +	path="$testroot/lib/$name"
> +	if test ! -h $path ; then
> +	    echo "error: missing file: $path"
> +	    errors=1
> +	fi
> +    done
> +}
> +
> +check_files
> +
> +${test_wrapper_env} \
> +${run_program_env} \
> +${common_objpfx}elf/ldconfig -X -f /dev/null \
> +  -C $testroot/etc/ld.so.cache \
> +  $testroot/lib
> +
> +check_files
> +
> +exit $errors
> +