From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 114162 invoked by alias); 15 Oct 2019 16:24:00 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 114144 invoked by uid 89); 15 Oct 2019 16:23:59 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=llp, During X-HELO: mx1.redhat.com Subject: Re: [PATCH] elf: Always set l in _dl_init_paths (Bug 23462). From: Carlos O'Donell To: libc-alpha Cc: Florian Weimer References: Message-ID: <916e86c1-dd59-6784-1318-856309aebd95@redhat.com> Date: Tue, 15 Oct 2019 16:24:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-10/txt/msg00437.txt.bz2 On 10/8/19 8:01 PM, Carlos O'Donell wrote: > I was doing a Fedora triage with Florian and we caught that this > issue had slipped through the cracks. Here is the patch with a > very simple test case. Ping. > 8< --- 8< --- 8< > After d1d5471579eb0426671bf94f2d71e61dfb204c30 we always setup the > link map l to make the static and shared cases the same. The bug > is that in elf/dl-load.c (_dl_init_paths) we conditionally set l > only in the #ifdef SHARED case, but unconditionally use it later. > The simple solution is to remove the #ifdef SHARED conditional, > because it's no longer needed, and unconditionally setup l for > both the static and shared cases. A regression test is added to > run a static binary with LD_LIBRARY_PATH='$ORIGIN' which crashes > before the fix and runs after the fix. > > Regression tested on x86_64. > --- > ChangeLog | 9 ++++++ > elf/Makefile | 5 +++- > elf/dl-load.c | 67 +++++++++++++++++++++----------------------- > elf/tst-dst-static.c | 30 ++++++++++++++++++++ > 4 files changed, 75 insertions(+), 36 deletions(-) > create mode 100644 elf/tst-dst-static.c > > diff --git a/ChangeLog b/ChangeLog > index 2ce48223f4..cf198257ba 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,12 @@ > +2019-10-08 Carlos O'Donell > + > + [BZ #23462] > + * elf/Makefile (tests-static-normal): tst-dst-static. > + (tst-dst-static-ENV): Define. > + * elf/dl-load.c (_dl_init_paths): Remove #ifdef SHARED, and refactor > + to always assume l is non-NULL. > + * elf/tst-dst-static.c: New file. > + > 2019-10-07 Carlos O'Donell > > * nptl/cancellation.c: Document that all functions here must be > diff --git a/elf/Makefile b/elf/Makefile > index dea51ca182..bb55baaa1f 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -148,7 +148,8 @@ endif > tests-static-normal := tst-leaks1-static tst-array1-static tst-array5-static \ > tst-dl-iter-static \ > tst-tlsalign-static tst-tlsalign-extern-static \ > - tst-linkall-static tst-env-setuid tst-env-setuid-tunables > + tst-linkall-static tst-env-setuid tst-env-setuid-tunables \ > + tst-dst-static > tests-static-internal := tst-tls1-static tst-tls2-static \ > tst-ptrguard1-static tst-stackguard1-static \ > tst-tls1-static-non-pie tst-libc_dlvsym-static > @@ -1557,3 +1558,5 @@ $(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o > $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so > > CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0 > + > +tst-dst-static-ENV = LD_LIBRARY_PATH='$$ORIGIN' > diff --git a/elf/dl-load.c b/elf/dl-load.c > index 438793e53d..903f8af13a 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -748,50 +748,47 @@ _dl_init_paths (const char *llp) > max_dirnamelen = SYSTEM_DIRS_MAX_LEN; > *aelem = NULL; > > -#ifdef SHARED > - /* This points to the map of the main object. */ > + /* This points to the map of the main object. It is always non-NULL > + since we have purposely made the dynamic and static cases look the > + same. */ > l = GL(dl_ns)[LM_ID_BASE]._ns_loaded; > - if (l != NULL) > + assert (l->l_type != lt_loaded); > + > + if (l->l_info[DT_RUNPATH]) > + { > + /* Allocate room for the search path and fill in information > + from RUNPATH. */ > + decompose_rpath (&l->l_runpath_dirs, > + (const void *) (D_PTR (l, l_info[DT_STRTAB]) > + + l->l_info[DT_RUNPATH]->d_un.d_val), > + l, "RUNPATH"); > + /* During rtld init the memory is allocated by the stub malloc, > + prevent any attempt to free it by the normal malloc. */ > + l->l_runpath_dirs.malloced = 0; > + > + /* The RPATH is ignored. */ > + l->l_rpath_dirs.dirs = (void *) -1; > + } > + else > { > - assert (l->l_type != lt_loaded); > + l->l_runpath_dirs.dirs = (void *) -1; > > - if (l->l_info[DT_RUNPATH]) > + if (l->l_info[DT_RPATH]) > { > /* Allocate room for the search path and fill in information > - from RUNPATH. */ > - decompose_rpath (&l->l_runpath_dirs, > + from RPATH. */ > + decompose_rpath (&l->l_rpath_dirs, > (const void *) (D_PTR (l, l_info[DT_STRTAB]) > - + l->l_info[DT_RUNPATH]->d_un.d_val), > - l, "RUNPATH"); > - /* During rtld init the memory is allocated by the stub malloc, > - prevent any attempt to free it by the normal malloc. */ > - l->l_runpath_dirs.malloced = 0; > - > - /* The RPATH is ignored. */ > - l->l_rpath_dirs.dirs = (void *) -1; > + + l->l_info[DT_RPATH]->d_un.d_val), > + l, "RPATH"); > + /* During rtld init the memory is allocated by the stub > + malloc, prevent any attempt to free it by the normal > + malloc. */ > + l->l_rpath_dirs.malloced = 0; > } > else > - { > - l->l_runpath_dirs.dirs = (void *) -1; > - > - if (l->l_info[DT_RPATH]) > - { > - /* Allocate room for the search path and fill in information > - from RPATH. */ > - decompose_rpath (&l->l_rpath_dirs, > - (const void *) (D_PTR (l, l_info[DT_STRTAB]) > - + l->l_info[DT_RPATH]->d_un.d_val), > - l, "RPATH"); > - /* During rtld init the memory is allocated by the stub > - malloc, prevent any attempt to free it by the normal > - malloc. */ > - l->l_rpath_dirs.malloced = 0; > - } > - else > - l->l_rpath_dirs.dirs = (void *) -1; > - } > + l->l_rpath_dirs.dirs = (void *) -1; > } > -#endif /* SHARED */ > > if (llp != NULL && *llp != '\0') > { > diff --git a/elf/tst-dst-static.c b/elf/tst-dst-static.c > new file mode 100644 > index 0000000000..7092f24a8e > --- /dev/null > +++ b/elf/tst-dst-static.c > @@ -0,0 +1,30 @@ > +/* Test DST expansion for static binaries doesn't carsh. Bug 23462. > + 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; if not, see > + . */ > + > +/* The purpose of this test is to exercise the code in elf/dl-loac.c > + (_dl_init_paths) or thereabout and ensure that static binaries > + don't crash when expanding DSTs. */ > +static int > +do_test (void) > +{ > + /* If the dynamic loader code linked into the static binary cannot > + handle expanding the DSTs e.g. null-deref on an incomplete link > + map, then it will crash before reaching main. */ > + return 0; > +} > +#include > -- Cheers, Carlos.