From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by sourceware.org (Postfix) with ESMTP id 6FC343857828 for ; Mon, 8 Mar 2021 22:26:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 6FC343857828 Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-548-gmusHgJGM82KZWks6aY1_w-1; Mon, 08 Mar 2021 17:26:43 -0500 X-MC-Unique: gmusHgJGM82KZWks6aY1_w-1 Received: by mail-qk1-f200.google.com with SMTP id 130so8531206qkm.0 for ; Mon, 08 Mar 2021 14:26:43 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=ZZL5zIhH8W5kSYJ0YUu/Uh3vkFfWLJtbSENdT0hmSKY=; b=SXAJ00uJ7SQY9pOkIC81ClLzHz+G1qdmYzAsF6tQ6AogoOV8TNaBUQopba5KZV/zde Pn1rOLvPUVlc1xrq7d4WTrKoDHpEeymicxPVI/7ddOjyPo+jMVPmVsr/68N9f7AOqisQ mwCdI4N2QxXV6krKyt/b67v3ggF83gkv7EeaReAFCkb1M7WxaC8XKWbaZ9PXkeFlcGVQ FjKLPx3CXu1dwtNzTgCCfu1augVNCXG7g9ow+C6SCg6bnO15U7+H7wH92YfdX7WLyIWC nT89CiSg/EHh7Zd2n180e0hJF/MzRRiK4hQwWa8oRbDmSs6wUO0YawDK4OE3PuARezsU wryg== X-Gm-Message-State: AOAM531rNuDwC07LR+1YCa4iPuBg7SHiL91RCWXd/zvDr4ifaoJyVAIs w6mn0GGm18qIBPx8WnP27sBeE26dSdJ5QOSmixntcDY6D6r1H44GDwaDxA56sL079FrRsUR4Kp7 70Od2+r+IStWNtjMNDZfRBB9V+KC6YamngNOH6sHbvNU1omoY+p3T5xaz08xVfVWWwTi8uQ== X-Received: by 2002:a37:396:: with SMTP id 144mr22477017qkd.412.1615242402660; Mon, 08 Mar 2021 14:26:42 -0800 (PST) X-Google-Smtp-Source: ABdhPJxxL91eDvH20GE4b3RJcVjr9rNHvXuBEkIJ8g2Nxeb9krRxa2b2eukixwZWTDvYCEuu/9flaw== X-Received: by 2002:a37:396:: with SMTP id 144mr22476994qkd.412.1615242402250; Mon, 08 Mar 2021 14:26:42 -0800 (PST) Received: from [192.168.1.16] (198-84-214-74.cpe.teksavvy.com. [198.84.214.74]) by smtp.gmail.com with ESMTPSA id 124sm8775632qkn.121.2021.03.08.14.26.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 08 Mar 2021 14:26:41 -0800 (PST) Subject: Re: [PATCH] elf: Always set l in _dl_init_paths (bug 23462) To: Florian Weimer , libc-alpha@sourceware.org References: <87h7lpbgpr.fsf@oldenburg.str.redhat.com> From: Carlos O'Donell Organization: Red Hat Message-ID: <725cc7f6-790e-93e4-623e-cc4186deb5ec@redhat.com> Date: Mon, 8 Mar 2021 17:26:40 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <87h7lpbgpr.fsf@oldenburg.str.redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Mar 2021 22:26:48 -0000 On 3/5/21 1:25 PM, Florian Weimer wrote: > From: Carlos O'Donell > > After d1d5471579eb0426671bf94f2d71e61dfb204c30 ("Remove dead > DL_DST_REQ_STATIC code.") 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. > > Co-Authored-By: Florian Weimer If you have reviewed this patch and find it sufficient this is enough consensus IMO to commit the patch. The technical direction was set by earlier patches where we are working towards making static and dynamic linking look and behave in similar ways. Thanks for taking this patch up, working through it, and adjusting the testing. Unless anyone objects I'd commit this patch since it fixes a real bug with ldconfig running when LD_LIBRRAY_PATH is set in some way globally. > --- > v2: Adjusted test not to use the test framework. Redid the change from > scratch. > > I think we should revisit testing for static DT_RUNPATH support > later, and not as part of this patch. > > elf/Makefile | 5 ++++- > elf/dl-load.c | 63 ++++++++++++++++++++++++---------------------------- > elf/tst-dst-static.c | 32 ++++++++++++++++++++++++++ > 3 files changed, 65 insertions(+), 35 deletions(-) > > diff --git a/elf/Makefile b/elf/Makefile > index b06bf6ca20..4c9e63dac9 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -164,7 +164,8 @@ 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-single_threaded-static tst-single_threaded-pthread-static > + tst-single_threaded-static tst-single_threaded-pthread-static \ > + tst-dst-static > > tests-static-internal := tst-tls1-static tst-tls2-static \ > tst-ptrguard1-static tst-stackguard1-static \ > @@ -1904,3 +1905,5 @@ $(objpfx)list-tunables.out: tst-rtld-list-tunables.sh $(objpfx)ld.so > cmp tst-rtld-list-tunables.exp \ > $(objpfx)/tst-rtld-list-tunables.out > $@; \ > $(evaluate-test) > + > +tst-dst-static-ENV = LD_LIBRARY_PATH='$$ORIGIN' > diff --git a/elf/dl-load.c b/elf/dl-load.c > index 9e2089cfaa..376a2e64d6 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -758,50 +758,45 @@ _dl_init_paths (const char *llp, const char *source, > max_dirnamelen = SYSTEM_DIRS_MAX_LEN; > *aelem = NULL; > > -#ifdef SHARED > /* This points to the map of the main object. */ > 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..56eb371c96 > --- /dev/null > +++ b/elf/tst-dst-static.c > @@ -0,0 +1,32 @@ > +/* Test DST expansion for static binaries doesn't carsh. Bug 23462. > + Copyright (C) 2021 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. > + > + 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, so the test harness > + is unnecessary. */ > + > +int > +main (void) > +{ > + return 0; > +} > -- Cheers, Carlos.