Re: [PATCH v3] elf: Add tst-ldconfig-bad-aux-cache test [BZ #18093]

[Carlos O'Donell <codonell at redhat dot com>]

On 5/13/19 9:34 AM, Alexandra Hájková wrote:
> From: Alexandra Hájková <ahajkova@redhat.com>
> 
> This test corrupts /var/cache/ldconfig/aux-cache and executes ldconfig
> to check it will not segfault using the corrupted aux_cache. The test
> uses the test-in-container framework. Verified no regressions on
> x86_64.

OK.

> 2019-05-13 Alexandra Hajkova  <ahajkova@redhat.com>
> 
>        * elf/Makefile (test-container): Add tst-ldconfig-bad-aux-cache.
>        * elf/tst-ldconfig-bad-aux-cache.c: New file.
>        * elf/tst-ldconfig_aux-cache.root: New directory.
>        * elf/tst-ldconfig-bad-aux-cache.root/postclean.req: New file
> 
>  v3
>  - use pid == 0 instead of !pid to follow glibc style rules
>  - call _exit after execve

Looking forward to v4 and a patch to add rootsbindir.

> ---
>  elf/Makefile                                  |   3 +
>  elf/tst-ldconfig-bad-aux-cache.c              | 136 ++++++++++++++++++
>  .../postclean.req                             |   0
>  3 files changed, 139 insertions(+)
>  create mode 100644 elf/tst-ldconfig-bad-aux-cache.c
>  create mode 100644 elf/tst-ldconfig-bad-aux-cache.root/postclean.req
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 4895489208..08e2f999bf 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -156,6 +156,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

OK.

> +
>  tests := tst-tls9 tst-leaks1 \
>  	tst-array1 tst-array2 tst-array3 tst-array4 tst-array5 \
>  	tst-auxv
> diff --git a/elf/tst-ldconfig-bad-aux-cache.c b/elf/tst-ldconfig-bad-aux-cache.c
> new file mode 100644
> index 0000000000..a1ed7a0877
> --- /dev/null
> +++ b/elf/tst-ldconfig-bad-aux-cache.c
> @@ -0,0 +1,136 @@
> +/* Test ldconfig does not segfault when aux-cache is corrupted (Bug 18093).

OK.

> +   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 <http://www.gnu.org/licenses/>.  */
> +
> +/* This test does the following:
> +   Run ldconfig to create the caches.
> +   Corrupt the caches.
> +   Run ldconfig again.
> +   At each step we verify that ldconfig does not crash.  */

OK.

> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <sys/wait.h>
> +#include <ftw.h>
> +#include <stdint.h>
> +
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/xunistd.h>
> +
> +#include <dirent.h>
> +
> +static int
> +display_info (const char *fpath, const struct stat *sb,
> +              int tflag, struct FTW *ftwbuf)
> +{
> +  printf ("%-3s %2d %7jd   %-40s %d %s\n",
> +          (tflag == FTW_D) ? "d" : (tflag == FTW_DNR) ? "dnr" :
> +          (tflag == FTW_DP) ? "dp" : (tflag == FTW_F) ? "f" :
> +          (tflag == FTW_NS) ? "ns" : (tflag == FTW_SL) ? "sl" :
> +          (tflag == FTW_SLN) ? "sln" : "???",
> +          ftwbuf->level, (intmax_t) sb->st_size,
> +          fpath, ftwbuf->base, fpath + ftwbuf->base);
> +  /* To tell nftw() to continue */
> +  return 0;
> +}

OK.

> +
> +/* Run ldconfig with a corrupt aux-cache, in particular we test for size
> +   truncation that might happen if a previous ldconfig run failed or if
> +   there were storage or power issues while we were writing the file.
> +   We want ldconfig not to crash, and it should be able to do so by
> +   computing the expected size of the file (bug 18093).  */
> +static int
> +do_test (void)
> +{
> +  char *args[] = { (char *) "/sbin/ldconfig", NULL };

We have a problem here which I didn't realize until we had this bug
reported: https://sourceware.org/bugzilla/show_bug.cgi?id=24544

Where we used to run /usr/bin/pldd, but with an alterate --prefix
it's not the correct path in the chroot. Users can also configure --bindir
also (part of the GNU Coding Standard).

The ldconfig binary is installed into the root sbin directory, and so
we should in the test use that directory to call ldconfig.

Therefore we need to fix this in the same way:

* In support/Makefile for the CFLAGS-support_paths.c rule add a new
  define e.g. -DROOTSBINDIR_PATH=\"$(rootsbindir)\"
* Add support_install_sbindir to support/support_paths.h which is defined as
  being SBINDIR_PATH.
* In your new test use strcpy/strcat to assemble the name of the test
  to call e.g. snprintf (prog, sizeof prog, "%s/pldd", support_install_rootsbindir) 

Want to submit a patch which just adds support_install_rootsbindir
and then submit v4 of this test and then I think we're done :-)

> +  const char *path = "/var/cache/ldconfig/aux-cache";

Interestingly enough I see a bug in glibc, not your test case though.

sysdeps/generic/ldconfig.h:

 48 /* Name of auxiliary cache.  */
 49 #define _PATH_LDCONFIG_AUX_CACHE "/var/cache/ldconfig/aux-cache"
 50 

However, we do allow users to override with --localstatedir and provide
an alternative path instead of the default /var. Nobody has probably
tried to do this or they just worked around it.

So the build of glibc itself will appear to ignore --localstatedir
even when specified and read the aux-cache from a fixed location.

This is not your problem, your test is fine.

> +  struct stat fs;
> +  long int size, new_size, i;
> +  int status;
> +  pid_t pid;
> +
> +  /* Create the needed directories. */
> +  xmkdirp ("/var/cache/ldconfig", 0777);

OK.

> +
> +  pid = xfork ();
> +  /* Run ldconfig fist to generate the aux-cache.  */
> +  if (pid == 0)
> +    {
> +      execv (args[0], args);
> +       _exit(1);

Spacing doesn't look right, please review.

> +    }
> +  else
> +    {
> +      if (pid)
> +        {
> +          xwaitpid (pid, &status, 0);
> +          if (!(WIFEXITED (status)))
> +            FAIL_EXIT1 ("ldconfig was aborted");
> +          if (stat (path, &fs) < 0)
> +          {
> +              if (errno == ENOENT)
> +                FAIL_EXIT1 ("aux-cache does not exist");
> +              else
> +                FAIL_EXIT1 ("Failed to open aux-cache.");
> +              return -1;
> +          }
> +
> +          size = fs.st_size;
> +          /* Run 3 tests, each truncating aux-cache shorter and shorter.  */
> +          for (i = 3; i > 0; i--)
> +            {
> +              new_size = size * i / 4;
> +              if (truncate (path, new_size))
> +              {
> +                  if (errno == ENOENT)
> +                      FAIL_EXIT1 ("aux-cache does not exist");
> +                  else
> +                      FAIL_EXIT1 ("truncation failed.");
> +                  return -1;
> +              }
> +              if (nftw (path, display_info, 1000, 0) == -1)
> +                {
> +                  FAIL_EXIT1 ("nftw failed.");
> +                  return -1;
> +                }
> +
> +              pid = xfork ();
> +              /* Verify that ldconfig can run with a truncated
> +                 aux-cache and doesn't crash.  */
> +              if (pid == 0)
> +                {
> +                  execv (args[0], args);
> +                  _exit(1);
> +                }
> +              else
> +                {
> +                  xwaitpid (pid, &status, 0);
> +                  if (!(WIFEXITED (status)))
> +                    FAIL_EXIT1 ("ldconfig exited with non-zero status");
> +                }
> +
> +            }
> +        }
> +    }
> +
> +  return 0;
> +}

OK.

> +
> +#include <support/test-driver.c>
> diff --git a/elf/tst-ldconfig-bad-aux-cache.root/postclean.req b/elf/tst-ldconfig-bad-aux-cache.root/postclean.req
> new file mode 100644
> index 0000000000..e69de29bb2

OK.

> 


-- 
Cheers,
Carlos.