| Message ID | 20190513133417.22988-1-ahajkova@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] elf: Add tst-ldconfig-bad-aux-cache test [BZ #18093] | expand |
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. >
On Mon, 13 May 2019, Carlos O'Donell wrote: > 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. There are lots of other places with such hardcoded paths. See e.g. bug 20138 asking for --sysconfdir to be respected. Note that some such paths are in installed headers, including paths.h; should those paths in installed headers also depend on configure options?
On 5/13/19 1:01 PM, Joseph Myers wrote: > On Mon, 13 May 2019, Carlos O'Donell wrote: > >> 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. > > There are lots of other places with such hardcoded paths. See e.g. bug > 20138 asking for --sysconfdir to be respected. Note that some such paths > are in installed headers, including paths.h; should those paths in > installed headers also depend on configure options? Would the _nl_default_dirname discussion set some precedent for this? https://sourceware.org/bugzilla/show_bug.cgi?id=14664 https://sourceware.org/ml/libc-alpha/2012-12/msg00192.html Namely that paths.h is not a part of the ABI, and therefore configure options can and should change the values in the header to match. The general agreement in _nl_default_dirname was: * Configure options should not change ABI. * Any option that changes ABI was likely a broken API design e.g. _nl_default_dirname and the generated COPY relocations it exposes are part of the ABI. If you want a substantially different ABI then I feel you should be creating a new target with it's own ABI.
On Mon, 13 May 2019, Carlos O'Donell wrote: > > There are lots of other places with such hardcoded paths. See e.g. bug > > 20138 asking for --sysconfdir to be respected. Note that some such paths > > are in installed headers, including paths.h; should those paths in > > installed headers also depend on configure options? > > Would the _nl_default_dirname discussion set some precedent for this? > > https://sourceware.org/bugzilla/show_bug.cgi?id=14664 > https://sourceware.org/ml/libc-alpha/2012-12/msg00192.html It's certainly relevant. > Namely that paths.h is not a part of the ABI, and therefore configure > options can and should change the values in the header to match. There's a general question here of when installed headers should depend on configure options - because when they do, that sets bounds on what multilib configurations can share a single set of installed headers. We expect multilib configurations for e.g. -m32 and -m64 to be able to share headers. Changing paths.h would imply that glibc builds with different paths can't share installed headers. (An edge case for the question of whether headers can be shared would be e.g. powerpc64 BE and LE - right now I think they can as there are no LE-specific headers, and GCC does have some multilib configurations with both BE and LE multilibs, but those configurations use their own lib* directory names nothing else in the toolchain knows about, so might not work well for other reasons.)
On 5/16/19 3:41 PM, Joseph Myers wrote: > On Mon, 13 May 2019, Carlos O'Donell wrote: > >>> There are lots of other places with such hardcoded paths. See e.g. bug >>> 20138 asking for --sysconfdir to be respected. Note that some such paths >>> are in installed headers, including paths.h; should those paths in >>> installed headers also depend on configure options? >> >> Would the _nl_default_dirname discussion set some precedent for this? >> >> https://sourceware.org/bugzilla/show_bug.cgi?id=14664 >> https://sourceware.org/ml/libc-alpha/2012-12/msg00192.html > > It's certainly relevant. > >> Namely that paths.h is not a part of the ABI, and therefore configure >> options can and should change the values in the header to match. > > There's a general question here of when installed headers should depend on > configure options - because when they do, that sets bounds on what > multilib configurations can share a single set of installed headers. We > expect multilib configurations for e.g. -m32 and -m64 to be able to share > headers. Changing paths.h would imply that glibc builds with different > paths can't share installed headers. (An edge case for the question of > whether headers can be shared would be e.g. powerpc64 BE and LE - right > now I think they can as there are no LE-specific headers, and GCC does > have some multilib configurations with both BE and LE multilibs, but those > configurations use their own lib* directory names nothing else in the > toolchain knows about, so might not work well for other reasons.) My opinion is that having multlibs share headers, particularly for -m32 and -m64 was a design mistake. We should have had completely distinct headers and keyed the search directory based on the machine being used. What historical need drove the requirement to share headers? Likewise my opinion is that two ABI-compatible toolchains, but compiled with distinct installed artifact locations, are two distinct runtimes, and they should have no expectation to be able to share headers.
On Thu, 16 May 2019, Carlos O'Donell wrote:
> What historical need drove the requirement to share headers?
My guess would be that it was more that multilib support (long before
sysroots) originally introduced a requirement for separate directories for
libraries without introducing such a requirement for headers. In that
context, it was natural to set up headers so they could be shared (with,
in glibc's case, special handling for gnu/lib-names.h and gnu/stubs.h).
Thus, there is in general in GCC no multilib-specific include directory
(no analogue of lib / lib64 / libx32 etc. directories for headers),
although it's possible to define SYSROOT_HEADERS_SUFFIX_SPEC (but most
configurations defining SYSROOT_SUFFIX_SPEC, possibly to an
automatically-generated definition based on the multilib configuration, do
not define SYSROOT_HEADERS_SUFFIX_SPEC).
On 5/16/19 4:35 PM, Joseph Myers wrote: > On Thu, 16 May 2019, Carlos O'Donell wrote: > >> What historical need drove the requirement to share headers? > > My guess would be that it was more that multilib support (long before > sysroots) originally introduced a requirement for separate directories for > libraries without introducing such a requirement for headers. In that > context, it was natural to set up headers so they could be shared (with, > in glibc's case, special handling for gnu/lib-names.h and gnu/stubs.h). > Thus, there is in general in GCC no multilib-specific include directory > (no analogue of lib / lib64 / libx32 etc. directories for headers), > although it's possible to define SYSROOT_HEADERS_SUFFIX_SPEC (but most > configurations defining SYSROOT_SUFFIX_SPEC, possibly to an > automatically-generated definition based on the multilib configuration, do > not define SYSROOT_HEADERS_SUFFIX_SPEC). This is exactly the answer I expected and in line with my own experience. In summary then, I think we should do no additional work to allow multiple differently configured glibc's to share the same headers, particularly /usr/include/paths.h (default install directory). If you wish to have differently configured installs then you will need a uniquely configured sysroot that is possibly triggered via a compiler spec file given a special option (reminds me of -msgxx-glibc in the CodeSourcery toolchains). I also think we should encourage the GNU Toolchain to move in a direction where the various multilib targets do not need to share headers. This sharing causes real problems in downstream where tooling cannot use "noarch" because the headers are really "only 2 arch" and then we need special support for i686 vs. x86_64 headers being interchangeable and correct. Basically the upstream GNU toolchain never considered how a downstream would package this kind of shared-header support.
On Thu, 16 May 2019, Carlos O'Donell wrote: > I also think we should encourage the GNU Toolchain to move in a direction > where the various multilib targets do not need to share headers. This sharing > causes real problems in downstream where tooling cannot use "noarch" because > the headers are really "only 2 arch" and then we need special support for > i686 vs. x86_64 headers being interchangeable and correct. Basically the > upstream GNU toolchain never considered how a downstream would package this > kind of shared-header support. There is certainly a case for more consistently ensuring that headers outside of bits/ and sys/ are architecture-independent (simply as a naming convention, independent of sharing installed header directories). Most of them are, but at least fpu_control.h and ieee754.h have architecture dependencies despite not being in one of the subdirectories normally used when there are such dependencies.
On 5/16/19 5:04 PM, Joseph Myers wrote: > On Thu, 16 May 2019, Carlos O'Donell wrote: > >> I also think we should encourage the GNU Toolchain to move in a direction >> where the various multilib targets do not need to share headers. This sharing >> causes real problems in downstream where tooling cannot use "noarch" because >> the headers are really "only 2 arch" and then we need special support for >> i686 vs. x86_64 headers being interchangeable and correct. Basically the >> upstream GNU toolchain never considered how a downstream would package this >> kind of shared-header support. > > There is certainly a case for more consistently ensuring that headers > outside of bits/ and sys/ are architecture-independent (simply as a naming > convention, independent of sharing installed header directories). Most of > them are, but at least fpu_control.h and ieee754.h have architecture > dependencies despite not being in one of the subdirectories normally used > when there are such dependencies. The purpose of such consistency is to reduce maintenance burden, and can be a natural consequence of such community activities, and from that perspective, you're right, it should be encouraged. However, there should be no explicit or implicit requirement to share headers, since there is no use case for it.
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 + 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). + 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. */ + +#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; +} + +/* 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 }; + const char *path = "/var/cache/ldconfig/aux-cache"; + struct stat fs; + long int size, new_size, i; + int status; + pid_t pid; + + /* Create the needed directories. */ + xmkdirp ("/var/cache/ldconfig", 0777); + + pid = xfork (); + /* Run ldconfig fist to generate the aux-cache. */ + if (pid == 0) + { + execv (args[0], args); + _exit(1); + } + 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; +} + +#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
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. 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 --- 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