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

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

Hi Alexandra,

> diff --git a/elf/Makefile b/elf/Makefile
> index 72a5aa88b1..a89226f163 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 \
> @@ -309,7 +310,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%,\
> @@ -1689,3 +1691,6 @@ $(objpfx)tst-dlopen-nodelete-reloc-mod17.so: \
>    $(objpfx)tst-dlopen-nodelete-reloc-mod15.so \
>    $(objpfx)tst-dlopen-nodelete-reloc-mod16.so
>  LDFLAGS-tst-dlopen-nodelete-reloc-mod17.so = -Wl,--no-as-needed
> +
> +$(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..f8f658d03b
> --- /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. I recall v1 had a whitespace issue; it's gone.

> 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..54fae5c829
> --- /dev/null
> +++ b/elf/tst-ldconfig-ld_so_conf-update.c
> @@ -0,0 +1,115 @@
> +/* Test ldconfig after /etc/ld.so.conf update and verify that a running process
> +   observes changes to /etc/ld.so.cache.

Looks okay.

> +   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. No changes here.

> +static void
> +run_ldconfig (void *x __attribute__((unused)))
> +{
> +  char *prog = xasprintf ("%s/ldconfig", support_install_rootsbindir);
> +  char *args[] = { prog, NULL };
> +
> +  execv (args[0], args);
> +  FAIL_EXIT1 ("execv: %m");
> +}

Function renamed and skipped using arguments. Looks okay.

> +/* 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.  */

Looks okay.

> +static int
> +do_test (void)
> +{
> +  struct support_capture_subprocess result;
> +
> +  /* Create the needed directories.  */
> +  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");
> +
> +
> +  /* Open the DSO.  We expect this to fail - tst-ldconfig directory
> +     is not searched.  */
> +  TEST_VERIFY_EXIT (dlopen (DSO, RTLD_NOW | RTLD_GLOBAL) == NULL);

You dropped the open_dso function in favour of TEST_VERIFY_EXIT. Looks okay.

> +  FILE *fp = xfopen (CONF, "a+");
> +  if (!fp)
> +    FAIL_EXIT1 ("creating /etc/ld.so.conf failed: %m");
> +  xfclose (fp);
> +
> +  /* Run ldconfig.  */
> +  result = support_capture_subprocess (run_ldconfig, NULL);
> +  support_capture_subprocess_check (&result, "execv", 0, sc_allow_none);
> +
> +  /* Try to dlopen the same DSO again, we expect this to fail again.  */
> +  TEST_VERIFY_EXIT (dlopen (DSO, RTLD_NOW | RTLD_GLOBAL) == NULL);
> +
> +  /* Add tst-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.  */
> +  TEST_VERIFY_EXIT (dlopen (DSO, RTLD_NOW | RTLD_GLOBAL) == NULL);
> +
> +  /* Run ldconfig again.  */
> +  result = support_capture_subprocess (run_ldconfig, NULL);
> +  support_capture_subprocess_check (&result, "execv", 0, sc_allow_none);
> +  support_capture_subprocess_free (&result);
> +
> +  /* Finally, we expect dlopen to pass now.  */
> +  TEST_VERIFY_EXIT (dlopen (DSO, RTLD_NOW | RTLD_GLOBAL) != NULL);
> +
> +  return 0;
> +}

Changes since v1 correspond to renaming function to run_ldconfig, and dropping
open_dso. Looks okay.

> +#include <support/test-driver.c>
> diff --git a/elf/tst-ldconfig-ld_so_conf-update.root/postclean.req b/elf/tst-ldconfig-ld_so_conf-update.root/postclean.req
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/elf/tst-ldconfig-ld_so_conf-update.root/tst-ldconfig-ld_so_conf-update.script b/elf/tst-ldconfig-ld_so_conf-update.root/tst-ldconfig-ld_so_conf-update.script
> new file mode 100644
> index 0000000000..be562ba554
> --- /dev/null
> +++ b/elf/tst-ldconfig-ld_so_conf-update.root/tst-ldconfig-ld_so_conf-update.script
> @@ -0,0 +1 @@
> +cp $B/elf/tst-ldconfig-ld-mod.so $L/tst-ldconfig-ld-mod.so

I applied the patch and ran the test. It ran and passed.

This version looks good to me and I'd say it's good enough for master.

Cheers,
Arjun