Re: [PATCH] elf: Add tst-ldconfig-ld_so_conf-update test

[Arjun Shankar <arjun dot is at lostca dot se>]

Hi Alexandra,

The patch looks good to me, overall.

I only have questions and nitpicks, which are as follows:

>  Test ldconfig after /etc/ld.so.conf update and verify a running process
>  observes changes to /etc/ld.so.cache.

Does this test for a bug that was reported and fixed at some point? If yes, is
there a bug number that can be included?

Also, looks like the intention of the test is to verify running processes'
obseration of cache changes, and not ldconfig itself. Am I right?

> diff --git a/elf/Makefile b/elf/Makefile
> index b2b3be203f..5a8d60c6a8 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -162,8 +162,9 @@ tests-static-internal := tst-tls1-static tst-tls2-static \
>  CRT-tst-tls1-static-non-pie := $(csu-objpfx)crt1.o
>  tst-tls1-static-non-pie-no-pie = yes
>  
> -tests-container = \
> -			  tst-ldconfig-bad-aux-cache
> +tests-container := \
> +			  tst-ldconfig-bad-aux-cache \
> +			  tst-ldconfig-ld_so_conf-update
>  
>  tests := tst-tls9 tst-leaks1 \
>  	tst-array1 tst-array2 tst-array3 tst-array4 tst-array5 \
> @@ -292,7 +293,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>  		tst-auditmanymod4 tst-auditmanymod5 tst-auditmanymod6 \
>  		tst-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9 \
>  		tst-initlazyfailmod tst-finilazyfailmod \
> -		tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2
> +		tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2 \
> +		tst-ldconfig-ld-mod
>  # Most modules build with _ISOMAC defined, but those filtered out
>  # depend on internal headers.
>  modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
> @@ -1627,3 +1629,6 @@ $(objpfx)tst-dlopenfailmod1.so: \
>    $(shared-thread-library) $(objpfx)tst-dlopenfaillinkmod.so
>  LDFLAGS-tst-dlopenfaillinkmod.so = -Wl,-soname,tst-dlopenfail-missingmod.so
>  $(objpfx)tst-dlopenfailmod2.so: $(shared-thread-library)
> +
> +$(objpfx)tst-ldconfig-ld_so_conf-update.out: $(objpfx)tst-ldconfig-ld-mod.so
> +$(objpfx)tst-ldconfig-ld_so_conf-update: $(libdl)

Looks okay.

> diff --git a/elf/tst-ldconfig-ld-mod.c b/elf/tst-ldconfig-ld-mod.c
> new file mode 100644
> index 0000000000..acd6609739
> --- /dev/null
> +++ b/elf/tst-ldconfig-ld-mod.c
> @@ -0,0 +1,8 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +void
> +bar (void)
> +{
> +      printf ("Called DSO.\n");
> +}

Looks okay minus glibc's two space indent.

> diff --git a/elf/tst-ldconfig-ld_so_conf-update.c b/elf/tst-ldconfig-ld_so_conf-update.c
> new file mode 100644
> index 0000000000..59de51c494
> --- /dev/null
> +++ b/elf/tst-ldconfig-ld_so_conf-update.c
> @@ -0,0 +1,143 @@
> +/* Test ldconfig after /etc/ld.so.conf update and verify a running process
> +   observes changes to /etc/ld.so.cache.
> +
> +   Copyright (C) 2019 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; see the file COPYING.LIB.  If
> +   not, see <https://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#include <support/capture_subprocess.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/xdlfcn.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
> +
> +#define DSO "libldconfig-ld-mod.so"
> +#define DSO_DIR "/tmp/tst-ldconfig"
> +#define CONF "/etc/ld.so.conf"

Looks okay.

> +static void
> +execv_wrapper (void *args)
> +{
> +  char **argv = args;
> +
> +  execv (argv[0], argv);
> +  FAIL_EXIT1 ("execv: %m");
> +}

Perhaps rename to 'run_ldconfig' and drop the use of args? This function is
only ever called to run ldconfig, and with the same args each time.

> +int
> +open_dso (const char *msg)
> +{
> +  void *dso;
> +  char *err;
> +  int ret = 0;
> +
> +  /* RTLD_NOW - all undefined symbols in the library are resolved
> +   * before dlopen() returns. If this cannot be done, an error
> +   * is returned.
> +   * RTLD_GLOBAL - The symbols defined by this library will be made
> +   * available for symbol resolution of subsequently loaded libraries.  */
> +  dso = dlopen (DSO, RTLD_NOW | RTLD_GLOBAL);
> +  if (dso == NULL)
> +    {
> +      err = dlerror ();
> +      printf ("%s dlerror: %s\n", msg, err);
> +      ret = 1;
> +    }
> +  /* Clear any errors.  */
> +  dlerror ();
> +
> +  return ret;
> +}

Since we aren't using the "expected" error messages in any way except
logging it, and since passing a string is a bit awkward, we could maybe
drop this entire function, and replace calls to it with:

(for expected failures)
TEST_VERIFY_EXIT (dlopen (DSO, RTLD_NOW | RTLD_GLOBAL) == NULL);

or

(for the final successful call)
TEST_VERIFY_EXIT (dlopen (DSO, RTLD_NOW | RTLD_GLOBAL) != NULL);

> +/* Create a new directory.
> +   Copy a test shared object there.
> +   Try to dlopen it by soname.  This should fail.
> +   (Directory is not searched.)
> +   Run ldconfig.
> +   Try to dlopen it again.  It should still fail.
> +   (Directory is still not searched.)
> +   Add the directory to /etc/ld.so.conf.
> +   Try to dlopen it again.  It should still fail.
> +   (The loader does not read /etc/ld.so.conf, only /etc/ld.so.cache.)
> +   Run ldconfig.
> +   Try to dlopen it again. This should finally succeed.  */

Description is clear. There's a missing double space before the last sentence.

> +static int
> +do_test (void)
> +{
> +  char *prog = xasprintf ("%s/ldconfig", support_install_rootsbindir);
> +  char *args[] = { prog, NULL };

Could go into 'run_ldconfig' (i.e. execv_wrapper).

> +  FILE *fp;
> +  struct support_capture_subprocess result;

Could be declared at point of first use.

> +  /* Create the needed directories. */

Missing double space.

> +  xmkdirp ("/var/cache/ldconfig", 0777);
> +  xmkdirp (DSO_DIR, 0777);
> +
> +  /* Rename the DSO to start with "lib" because there's an undocumented
> +     filter in ldconfig where it ignores any file that doesn't start with
> +     "lib" (for regular shared libraries) or "ld-" (for ld-linux-*).  */
> +  if (rename ("/usr/lib64/tst-ldconfig-ld-mod.so",
> +              "/tmp/tst-ldconfig/libldconfig-ld-mod.so"))
> +    FAIL_EXIT1 ("Renaming/moving the DSO failed: %m");

Looks okay.

> +  /* Open the DSO. We expect this to fail - test_ldconfig directory
> +   * is not searched.  */

The directory you used is tst-ldconfig.

> +  if (!open_dso ("[Expected] "))
> +    FAIL_EXIT1
> +      ("We expect dlopen to fail at this point - test dir is not searched.");

Could be replaced with that line I suggested earlier:
TEST_VERIFY_EXIT (dlopen (DSO, RTLD_NOW | RTLD_GLOBAL) == NULL);

> +  fp = xfopen (CONF, "a+");
> +  if (!fp)
> +    FAIL_EXIT1 ("creating /etc/ld.so.conf failed: %m");
> +  xfclose (fp);

Looks okay.

> +  /* Run ldconfig.  */
> +  result = support_capture_subprocess (execv_wrapper, args);
> +  support_capture_subprocess_check (&result, "execv", 0, sc_allow_none);
> +
> +  /* Try to dlopen the same DSO again, we expect this to fail again.  */
> +  if (!open_dso ("[Expected] "))
> +    FAIL_EXIT1 ("We expect dlopen to fail at this point!.");
> +
> +  /* Add test_ldconfig directory to /etc/ld.so.conf.  */
> +  fp = xfopen (CONF, "w");
> +  if (!(fwrite (DSO_DIR, 1, sizeof (DSO_DIR), fp)))
> +    FAIL_EXIT1 ("updating /etc/ld.so.conf failed: %m");
> +  xfclose (fp);
> +
> +  /* Try to dlopen the same DSO again, we expect this to still fail.  */
> +  if (!open_dso ("[Expected] "))
> +    FAIL_EXIT1 ("We expect dlopen to fail at this point!.");
> +
> +  /* Run ldconfig again.  */
> +  result = support_capture_subprocess (execv_wrapper, args);
> +  support_capture_subprocess_check (&result, "execv", 0, sc_allow_none);
> +  support_capture_subprocess_free (&result);
> +
> +  if (open_dso ("[Unexpected] "))
> +    FAIL_EXIT1 ("Dlopen failed.");

Looks okay minus the one reference to test_ldconfig. Of course, it will change a
bit if you go for my dlopen and run_ldconfig suggestions.

Cheers,
Arjun