From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 20083 invoked from network); 11 Nov 2020 00:52:36 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 11 Nov 2020 00:52:36 -0000 Received: (qmail 5274 invoked by uid 550); 11 Nov 2020 00:52:31 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 5244 invoked from network); 11 Nov 2020 00:52:30 -0000 Date: Tue, 10 Nov 2020 19:52:17 -0500 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20201111005216.GH534@brightrain.aerifal.cx> References: <20201027211735.GV534@brightrain.aerifal.cx> <20201030185205.GA10849@arya.arvanta.net> <20201030185716.GE534@brightrain.aerifal.cx> <5298816.XTEcGr0bgB@nanabozho> <20201031033117.GH534@brightrain.aerifal.cx> <20201106033616.GX534@brightrain.aerifal.cx> <20201108161215.GE1370092@port70.net> <20201109170729.GA534@brightrain.aerifal.cx> <20201109215901.GG1370092@port70.net> <20201109222320.GC534@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="OX2aLCKeO1apYW07" Content-Disposition: inline In-Reply-To: <20201109222320.GC534@brightrain.aerifal.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH v2] MT fork --OX2aLCKeO1apYW07 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Nov 09, 2020 at 05:23:21PM -0500, Rich Felker wrote: > On Mon, Nov 09, 2020 at 10:59:01PM +0100, Szabolcs Nagy wrote: > > (not ideal, since then interposers can't see all > > allocations, which some tools would like to see, > > but at least correct and robust. and it is annoying > > that we have to do all this extra work just because > > of mt-fork) > > Yes. On the other hand if this were done more rigorously it would fix > the valgrind breakage of malloc during ldso startup.. > > > > The other pros of such an approach are stuff like making it so > > > application code doesn't get called as a callback from messy contexts > > > inside libc, e.g. with dynamic linker in inconsistent state. The major > > > con I see is that it precludes omitting the libc malloc entirely when > > > static linking, assuming you link any part of libc that uses malloc > > > internally. However, almost all such places only call malloc, not > > > free, so you'd just get the trivial bump allocator gratuitously > > > linked, rather than full mallocng or oldmalloc, except for dlerror > > > which shouldn't come up in static linked programs anyway. > > > > i see. > > that sounds fine to me. > > I'm still not sure it's fine, so I appreciate your input and anyone > else's who has spent some time thinking about this. Here's a proposed first patch in series, getting rid of getdelim/stdio usage in ldso. I think that suffices to set the stage for adding __libc_malloc, __libc_free, __libc_calloc, __libc_realloc and having ldso use them. To make this work, I think malloc needs to actually be a separate function wrapping __libc_malloc -- this is because __simple_malloc precludes the malloc symbol itself being weak. That's a very slight runtime cost, but has the benefit of eliminating the awful hack of relyin on link order to get __simple_malloc (bump allocator) to be chosen over full malloc. Now, the source file containing the bump allocator can define malloc as a call to __libc_malloc, and provide the bump allocator as the weak definition of __libc_malloc. mallocng/malloc.c would then provide the strong definition of __libc_malloc. For the other functions, I think __libc_* can theoretically just be aliases for the public symbols, but this may (almost surely does) break valgrind, and it's messy to do at the source level, so perhaps they should be wrapped too. This should entirely prevent valgrind from interposing the libc-internal calls, thereby fixing the longstanding bug where it crashes by interposing them too early. Rich --OX2aLCKeO1apYW07 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0001-drop-use-of-getdelim-stdio-in-dynamic-linker.patch" >From b24186676cbc87c0e2c3e0fa672ef73ea55b600e Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Tue, 10 Nov 2020 19:32:09 -0500 Subject: [PATCH] drop use of getdelim/stdio in dynamic linker the only place stdio was used here was for reading the ldso path file, taking advantage of getdelim to automatically allocate and resize the buffer. the motivation for use here was that, with shared libraries, stdio is already available anyway and free to use. this has long been a nuisance to users because getdelim's use of realloc here triggered a valgrind bug, but removing it doesn't really fix that; on some archs even calling the valgrind-interposed malloc at this point will crash. the actual motivation for this change is moving towards getting rid of use of application-provided malloc in parts of libc where it would be called with libc-internal locks held, leading to the possibility of deadlock if the malloc implementation doesn't follow unwritten rules about which libc functions are safe for it to call. since getdelim is required to produce a pointer as if by malloc (i.e. that can be passed to reallor or free), it necessarily must use the public malloc. instead of performing a realloc loop as the path file is read, first query its size with fstat and allocate only once. this produces slightly different truncation behavior when racing with writes to a file, but neither behavior is or could be made safe anyway; on a live system, ldso path files should be replaced by atomic rename only. the change should also reduce memory waste. --- ldso/dynlink.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/ldso/dynlink.c b/ldso/dynlink.c index f9ac0100..d736bf12 100644 --- a/ldso/dynlink.c +++ b/ldso/dynlink.c @@ -1,6 +1,5 @@ #define _GNU_SOURCE #define SYSCALL_NO_TLS 1 -#include #include #include #include @@ -556,6 +555,20 @@ static void reclaim_gaps(struct dso *dso) } } +static ssize_t read_loop(int fd, void *p, size_t n) +{ + unsigned char *b = p; + for (size_t l, i=0; i=0) { + size_t n = 0; + if (!fstat(fd, &st)) n = st.st_size; + sys_path = malloc(n+1); + sys_path[n] = 0; + if (!sys_path || read_loop(fd, sys_path, n)<0) { free(sys_path); sys_path = ""; } - fclose(f); + close(fd); } else if (errno != ENOENT) { sys_path = ""; } -- 2.21.0 --OX2aLCKeO1apYW07--