From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from camel.birch.relay.mailchannels.net (camel.birch.relay.mailchannels.net [23.83.209.29]) by sourceware.org (Postfix) with ESMTPS id 608B73858C83 for ; Tue, 26 Apr 2022 18:44:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 608B73858C83 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gotplt.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gotplt.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 795AC5A13E8; Tue, 26 Apr 2022 18:44:07 +0000 (UTC) Received: from pdx1-sub0-mail-a307.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 73ABB5A2168; Tue, 26 Apr 2022 18:44:05 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1650998645; a=rsa-sha256; cv=none; b=y7DaJGU085gbexzR/PE3mzjgK4flTmM8bYpxnj/G1tH5Nunfl+UUD3hZvFG7gxiJrkXcCe mvVn3xUOVfgbT86xug1JlRLlOeN7ForFusEMJenS88IwecyArFsMGK6Laby2rbl16JfSZ2 T3q9jFbwju3a8d0ToyKSIdel+Vkqsu36ohuYRZwVptqhJBaS0KJYrg9awASjKUHzKCub0T glrb83MzIluqG0HAKuEYFEz9cUPglbb9sjTbwgikfD3fG768Twy+8ndxsa+GDFash7hRtY eUZxhj4vURtM31K+2piMwRX1eUwrjIYBRrpPcd6ymJpg0Y5A7saQ0+hpzASXkQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1650998645; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=7/WoqAFzqhuF25eKcmLRc/6LDhOPXLRwdOnoS3MdU3c=; b=01c+xax6XGnvqX6m5J1ApJ4qlfic/gawgzbqNya0wYYhjYDpT3YLx7ePG6SEevXi8Bsr8U nl2K4WvDohhJaETDpUqWbEtSapxdLT0O4yKD97M1JCP0stWwz9pdfPvZxecg9ngvSA1OXa VJO/4U2NtUAlLZ0Dona9TkuPrdIEqZGykNSf9Ezl1OV02Xc82ZfTAwQLb42lmEo7KlVDfY VIlqM6UM85aMBl7PnNkisYDj9WepQwRpEdGFdp570K9+2FeoBdCTqrAK6IEZ9MvHGA/JvX /VLir8kBpd3ofAUlKO3SEd5/uujUGr0u1hVnG3uA/YPBcWjXlTcGVF6R0ogjiA== ARC-Authentication-Results: i=1; rspamd-6dfbdcb948-jk4df; auth=pass smtp.auth=dreamhost smtp.mailfrom=siddhesh@gotplt.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from pdx1-sub0-mail-a307.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.125.123.37 (trex/6.7.1); Tue, 26 Apr 2022 18:44:07 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Interest-Reign: 262f889b61e226e9_1650998646786_3626201732 X-MC-Loop-Signature: 1650998646786:3754464404 X-MC-Ingress-Time: 1650998646786 Received: from [192.168.1.174] (unknown [1.186.121.227]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a307.dreamhost.com (Postfix) with ESMTPSA id 4KnrPZ6sDczdC; Tue, 26 Apr 2022 11:43:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gotplt.org; s=dreamhost; t=1650998628; bh=7/WoqAFzqhuF25eKcmLRc/6LDhOPXLRwdOnoS3MdU3c=; h=Date:Subject:To:From:Content-Type:Content-Transfer-Encoding; b=XLdsROHyBu3Gz76GUucKtF5KKMMqBSa0fuhu3SXkor+5+RxrGhVwzaUQrDj4Cg7xR seWPc87KCKT9977qcWU0J+f5hBgKDXMjOfyIs05qADtduzXALJzsM1Sob46b6REZWo B8b7o3VzApOkAxXqrk8NX6Hcrc34skBdgRE8ILCvGa3uOZUE3pNj8bHt2fjt6/K7Et uiiqUxks8fLWBXRNLxFCjRZycMtbCTIJWUGNz3RaW1GgSe336JZ6jcMER1mxyTIIWK WUsWpXdhLO8iU5f1j80OlgtUM2lo3StOEczBDJRRXZk0jWJ44g8PSpZspFTEgRXPOF SrcMX9Xmn//2g== Message-ID: <594f5e59-699b-627c-5858-68a4fb3ce51d@gotplt.org> Date: Wed, 27 Apr 2022 00:13:41 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH v5] elf: Fix DFS sorting algorithm for LD_TRACE_LOADED_OBJECTS with missing libraries (BZ #28868) Content-Language: en-US To: Adhemerval Zanella , libc-alpha@sourceware.org, Florian Weimer References: <20220304171200.3101821-1-adhemerval.zanella@linaro.org> From: Siddhesh Poyarekar In-Reply-To: <20220304171200.3101821-1-adhemerval.zanella@linaro.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3038.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_SBL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Tue, 26 Apr 2022 18:44:12 -0000 The patch doesn't apply anymore (conflict in elf/Makefile), please rebase it and post. More comments below: On 04/03/2022 22:42, Adhemerval Zanella via Libc-alpha wrote: > The _dl_map_object_deps ignores l_faked objects (set if the underlying > file can't be opened by _dl_map_object): Maybe a more precise description is that the underlying file is not opened by _dl_map_object and the linker is in trace mode? In other cases where the underlying file can't be opened, _dl_map_object quits with an error. > > 490 for (nlist = 0, runp = known; runp; runp = runp->next) > 491 { > 492 if (__builtin_expect (trace_mode, 0) && runp->map->l_faked) > 493 /* This can happen when we trace the loading. */ > 494 --map->l_searchlist.r_nlist; > 495 else > 496 { > 497 if (runp->map == map) > 498 map_index = nlist; > 499 map->l_searchlist.r_list[nlist++] = runp->map; > 500 } > 501 > 502 /* Now clear all the mark bits we set in the objects on the search list > 503 to avoid duplicates, so the next call starts fresh. */ > 504 runp->map->l_reserved = 0; > 505 } > > If there any missing libraries being processed, they will not be > considered on final nlist size passed on _dl_sort_maps later in the > function. And it is then used on _dl_sort_maps_dfs on the stack > allocated working maps: > > 222 /* Array to hold RPO sorting results, before we copy back to maps[]. */ > 223 struct link_map *rpo[nmaps]; > 224 > 225 /* The 'head' position during each DFS iteration. Note that we start at > 226 one past the last element due to first-decrement-then-store (see the > 227 bottom of above dfs_traversal() routine). */ > 228 struct link_map **rpo_head = &rpo[nmaps]; > > However while transversing the 'l_initfini' on dfs_traversal it will > still considere the l_faked maps and thus update rpo more times than the > allocated working 'rpo', overflowing the stack object. > > As suggested in bugzilla, one option would be to avoid sorting the maps > for trace mode. However I think ignoring l_faked object does make > sense (there is one less constraint to call the sorting function), it > allows a slight less stack usage for trace, and it is slight simpler > solution. > > The tests does trigger the stack overflow, however I tried to make > it more generic to check different scenarios or missing objects. > > Checked on x86_64-linux-gnu. > --- > v5: Refactor tests make rules. > v4: Use --soname to build the libraries, fixed typos. > v3: Removed stamp files to avoid add them as linker depedencies and > moved test evaluation to python script. > v2: Added comments and fixed the default tst-trace1 --library-path. > --- > elf/Makefile | 54 ++++++++++++++++++++ > elf/dl-deps.c | 2 + > elf/dl-sort-maps.c | 4 +- > elf/libtracemod1-1.c | 1 + > elf/libtracemod2-1.c | 1 + > elf/libtracemod3-1.c | 1 + > elf/libtracemod4-1.c | 1 + > elf/libtracemod5-1.c | 1 + > elf/libtracemod6.c | 1 + > elf/tst-trace1.exp | 4 ++ > elf/tst-trace2.exp | 6 +++ > elf/tst-trace3.exp | 6 +++ > elf/tst-trace4.exp | 6 +++ > elf/tst-trace5.exp | 6 +++ > scripts/tst-ld-trace.py | 108 ++++++++++++++++++++++++++++++++++++++++ > 15 files changed, 201 insertions(+), 1 deletion(-) > create mode 100644 elf/libtracemod1-1.c > create mode 100644 elf/libtracemod2-1.c > create mode 100644 elf/libtracemod3-1.c > create mode 100644 elf/libtracemod4-1.c > create mode 100644 elf/libtracemod5-1.c > create mode 100644 elf/libtracemod6.c > create mode 100644 elf/tst-trace1.exp > create mode 100644 elf/tst-trace2.exp > create mode 100644 elf/tst-trace3.exp > create mode 100644 elf/tst-trace4.exp > create mode 100644 elf/tst-trace5.exp > create mode 100755 scripts/tst-ld-trace.py > > diff --git a/elf/Makefile b/elf/Makefile > index c96924e9c2..f30f64991f 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -617,6 +617,11 @@ modules-names = \ > libmarkermod4-2 \ > libmarkermod4-3 \ > libmarkermod4-4 \ > + libtracemod1-1 \ > + libtracemod2-1 \ > + libtracemod3-1 \ > + libtracemod4-1 \ > + libtracemod5-1 \ > ltglobmod1 \ > ltglobmod2 \ > neededobj1 \ > @@ -1079,6 +1084,11 @@ tests-special += \ > $(objpfx)tst-initorder2-cmp.out \ > $(objpfx)tst-unused-dep-cmp.out \ > $(objpfx)tst-unused-dep.out \ > + $(objpfx)tst-trace1.out \ > + $(objpfx)tst-trace2.out \ > + $(objpfx)tst-trace3.out \ > + $(objpfx)tst-trace4.out \ > + $(objpfx)tst-trace5.out \ > # tests-special > endif > > @@ -2725,3 +2735,47 @@ $(objpfx)tst-p_align3: $(objpfx)tst-p_alignmod3.so > $(objpfx)tst-p_align3.out: tst-p_align3.sh $(objpfx)tst-p_align3 > $(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \ > $(evaluate-test) > + > +LDFLAGS-libtracemod1-1.so += -Wl,-soname,libtracemod1.so > +LDFLAGS-libtracemod2-1.so += -Wl,-soname,libtracemod2.so > +LDFLAGS-libtracemod3-1.so += -Wl,-soname,libtracemod3.so > +LDFLAGS-libtracemod4-1.so += -Wl,-soname,libtracemod4.so > +LDFLAGS-libtracemod5-1.so += -Wl,-soname,libtracemod5.so > + > +$(objpfx)libtracemod1-1.so: $(objpfx)libtracemod2-1.so \ > + $(objpfx)libtracemod3-1.so > +$(objpfx)libtracemod2-1.so: $(objpfx)libtracemod4-1.so \ > + $(objpfx)libtracemod5-1.so > + > +define libtracemod-x > +$(objpfx)libtracemod$(1)/libtracemod$(1).so: $(objpfx)libtracemod$(1)-1.so > + $$(make-target-directory) > + cp $$< $$@ > +endef > +libtracemod-suffixes = 1 2 3 4 5 > +$(foreach i,$(libtracemod-suffixes), $(eval $(call libtracemod-x,$(i)))) > + > +define tst-trace-skeleton > +$(objpfx)tst-trace$(1).out: $(objpfx)libtracemod1/libtracemod1.so \ > + $(objpfx)libtracemod2/libtracemod2.so \ > + $(objpfx)libtracemod3/libtracemod3.so \ > + $(objpfx)libtracemod4/libtracemod4.so \ > + $(objpfx)libtracemod5/libtracemod5.so \ > + $(..)scripts/tst-ld-trace.py \ > + tst-trace$(1).exp > + ( $(PYTHON) $(..)scripts/tst-ld-trace.py \ > + "$(test-wrapper-env) $(elf-objpfx)$(rtld-installed-name) \ > + --library-path $(common-objpfx):$(strip $(2)) \ > + $(objpfx)libtracemod1/libtracemod1.so" tst-trace$(1).exp \ > + ) > $$@; $$(evaluate-test) > +endef OK, but would it be possible to not spawn the script as a subprocess? i.e. use {} instead of () for the recipe. > + > +$(eval $(call tst-trace-skeleton,1,)) > +$(eval $(call tst-trace-skeleton,2,\ > + $(objpfx)libtracemod2)) > +$(eval $(call tst-trace-skeleton,3,\ > + $(objpfx)libtracemod2:$(objpfx)libtracemod3)) > +$(eval $(call tst-trace-skeleton,4,\ > + $(objpfx)libtracemod2:$(objpfx)libtracemod3:$(objpfx)libtracemod4)) > +$(eval $(call tst-trace-skeleton,5,\ > + $(objpfx)libtracemod2:$(objpfx)libtracemod3:$(objpfx)libtracemod4:$(objpfx)libtracemod5)) OK. > diff --git a/elf/dl-deps.c b/elf/dl-deps.c > index a2fc278256..77ff2b3895 100644 > --- a/elf/dl-deps.c > +++ b/elf/dl-deps.c > @@ -473,6 +473,8 @@ _dl_map_object_deps (struct link_map *map, > > for (nlist = 0, runp = known; runp; runp = runp->next) > { > + /* _dl_sort_maps ignores l_faked object, so it is safe to not considere consider > + them for nlist. */ > if (__builtin_expect (trace_mode, 0) && runp->map->l_faked) > /* This can happen when we trace the loading. */ > --map->l_searchlist.r_nlist; > diff --git a/elf/dl-sort-maps.c b/elf/dl-sort-maps.c > index 9e9d53ec47..96638d7ed1 100644 > --- a/elf/dl-sort-maps.c > +++ b/elf/dl-sort-maps.c > @@ -140,7 +140,9 @@ static void > dfs_traversal (struct link_map ***rpo, struct link_map *map, > bool *do_reldeps) > { > - if (map->l_visited) > + /* _dl_map_object_deps ignores l_faked objects when calculating the > + number of maps before calling _dl_sort_maps, ignore them as well. */ > + if (map->l_visited || map->l_faked) > return; > > map->l_visited = 1; OK. > diff --git a/elf/libtracemod1-1.c b/elf/libtracemod1-1.c > new file mode 100644 > index 0000000000..7c89c9a5a4 > --- /dev/null > +++ b/elf/libtracemod1-1.c > @@ -0,0 +1 @@ > +/* Empty */ > diff --git a/elf/libtracemod2-1.c b/elf/libtracemod2-1.c > new file mode 100644 > index 0000000000..7c89c9a5a4 > --- /dev/null > +++ b/elf/libtracemod2-1.c > @@ -0,0 +1 @@ > +/* Empty */ > diff --git a/elf/libtracemod3-1.c b/elf/libtracemod3-1.c > new file mode 100644 > index 0000000000..7c89c9a5a4 > --- /dev/null > +++ b/elf/libtracemod3-1.c > @@ -0,0 +1 @@ > +/* Empty */ > diff --git a/elf/libtracemod4-1.c b/elf/libtracemod4-1.c > new file mode 100644 > index 0000000000..7c89c9a5a4 > --- /dev/null > +++ b/elf/libtracemod4-1.c > @@ -0,0 +1 @@ > +/* Empty */ > diff --git a/elf/libtracemod5-1.c b/elf/libtracemod5-1.c > new file mode 100644 > index 0000000000..7c89c9a5a4 > --- /dev/null > +++ b/elf/libtracemod5-1.c > @@ -0,0 +1 @@ > +/* Empty */ > diff --git a/elf/libtracemod6.c b/elf/libtracemod6.c > new file mode 100644 > index 0000000000..7c89c9a5a4 > --- /dev/null > +++ b/elf/libtracemod6.c > @@ -0,0 +1 @@ > +/* Empty */ Isn't this unused? > diff --git a/elf/tst-trace1.exp b/elf/tst-trace1.exp > new file mode 100644 > index 0000000000..4a6f5211a6 > --- /dev/null > +++ b/elf/tst-trace1.exp > @@ -0,0 +1,4 @@ > +ld 1 > +libc 1 > +libtracemod2.so 0 > +libtracemod3.so 0 OK, since libtracemod1.so depends on libtracemod2.so and libtracemod3.so but they're not in path. > diff --git a/elf/tst-trace2.exp b/elf/tst-trace2.exp > new file mode 100644 > index 0000000000..e13506e2eb > --- /dev/null > +++ b/elf/tst-trace2.exp > @@ -0,0 +1,6 @@ > +ld 1 > +libc 1 > +libtracemod2.so 1 > +libtracemod3.so 0 > +libtracemod4.so 0 > +libtracemod5.so 0 OK, since libtracemod2.so is now in path and libtracemod4.so and libtracemod5.so get pulled in libtracemod2.so. > diff --git a/elf/tst-trace3.exp b/elf/tst-trace3.exp > new file mode 100644 > index 0000000000..e574549d12 > --- /dev/null > +++ b/elf/tst-trace3.exp > @@ -0,0 +1,6 @@ > +ld 1 > +libc 1 > +libtracemod2.so 1 > +libtracemod3.so 1 > +libtracemod4.so 0 > +libtracemod5.so 0 OK, since libtracemod3 is also in path. > diff --git a/elf/tst-trace4.exp b/elf/tst-trace4.exp > new file mode 100644 > index 0000000000..31ca97b35b > --- /dev/null > +++ b/elf/tst-trace4.exp > @@ -0,0 +1,6 @@ > +ld 1 > +libc 1 > +libtracemod2.so 1 > +libtracemod3.so 1 > +libtracemod4.so 1 > +libtracemod5.so 0 OK, since libtracemod4 is also in path. > diff --git a/elf/tst-trace5.exp b/elf/tst-trace5.exp > new file mode 100644 > index 0000000000..5d7d953726 > --- /dev/null > +++ b/elf/tst-trace5.exp > @@ -0,0 +1,6 @@ > +ld 1 > +libc 1 > +libtracemod2.so 1 > +libtracemod3.so 1 > +libtracemod4.so 1 > +libtracemod5.so 1 OK, since libtracemod5 is also in path. > diff --git a/scripts/tst-ld-trace.py b/scripts/tst-ld-trace.py > new file mode 100755 > index 0000000000..f5a4028003 > --- /dev/null > +++ b/scripts/tst-ld-trace.py > @@ -0,0 +1,108 @@ > +#!/usr/bin/python3 > +# Dump the output of LD_TRACE_LOADED_OBJECTS in architecture neutral format. > +# Copyright (C) 2022 Free Software Foundation, Inc. > +# Copyright The GNU Toolchain Authors. > +# 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 > +# . > + > +import argparse > +import os > +import subprocess > +import sys > + > +try: > + subprocess.run > +except: > + class _CompletedProcess: > + def __init__(self, args, returncode, stdout=None, stderr=None): > + self.args = args > + self.returncode = returncode > + self.stdout = stdout > + self.stderr = stderr > + > + def _run(*popenargs, input=None, timeout=None, check=False, **kwargs): > + assert(timeout is None) > + with subprocess.Popen(*popenargs, **kwargs) as process: > + try: > + stdout, stderr = process.communicate(input) > + except: > + process.kill() > + process.wait() > + raise > + returncode = process.poll() > + if check and returncode: > + raise subprocess.CalledProcessError(returncode, popenargs) > + return _CompletedProcess(popenargs, returncode, stdout, stderr) > + > + subprocess.run = _run > + > +def is_vdso(lib): > + return lib.startswith('linux-gate') or lib.startswith('linux-vdso') OK. > + > + > +def parse_trace(cmd, fref): > + new_env = os.environ.copy() > + new_env['LD_TRACE_LOADED_OBJECTS'] = '1' > + trace_out = subprocess.run(cmd, stdout=subprocess.PIPE, check=True, > + universal_newlines=True, env=new_env).stdout > + trace = [] > + for line in trace_out.splitlines(): > + line = line.strip() > + if is_vdso(line): > + continue > + fields = line.split('=>' if '=>' in line else ' ') > + lib = os.path.basename(fields[0].strip()) > + if lib.startswith('ld'): > + lib = 'ld' > + elif lib.startswith('libc'): > + lib = 'libc' > + found = 1 if fields[1].strip() != 'not found' else 0 > + trace += ['{} {}'.format(lib, found)] > + trace = sorted(trace) > + > + reference = sorted(line.replace('\n','') for line in fref.readlines()) > + > + ret = 0 if trace == reference else 1 > + if ret != 0: > + for i in reference: > + if i not in trace: > + print("Only in {}: {}".format(fref.name, i)) > + for i in trace: > + if i not in reference: > + print("Only in trace: {}".format(i)) > + > + sys.exit(ret) OK. > + > + > +def get_parser(): > + parser = argparse.ArgumentParser(description=__doc__) > + parser.add_argument('command', > + help='comand to run') > + parser.add_argument('reference', > + help='reference file to compare') > + return parser > + OK. > + > +def main(argv): > + parser = get_parser() > + opts = parser.parse_args(argv) > + with open(opts.reference, 'r') as fref: > + # Remove the initial 'env' command. > + parse_trace(opts.command.split()[1:], fref) > + > + > +if __name__ == '__main__': > + main(sys.argv[1:]) OK.