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 2541 invoked from network); 21 Jan 2021 16:27:51 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 21 Jan 2021 16:27:51 -0000 Received: (qmail 7377 invoked by uid 550); 21 Jan 2021 16:27:47 -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 7351 invoked from network); 21 Jan 2021 16:27:46 -0000 Date: Thu, 21 Jan 2021 11:27:34 -0500 From: Rich Felker To: "Alex Xu (Hello71)" Cc: musl@lists.openwall.com Message-ID: <20210121162734.GO23432@brightrain.aerifal.cx> References: <20210121140240.83405-1-alex_y_xu.ref@yahoo.ca> <20210121140240.83405-1-alex_y_xu@yahoo.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210121140240.83405-1-alex_y_xu@yahoo.ca> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] don't set errno in free On Thu, Jan 21, 2021 at 09:02:40AM -0500, Alex Xu (Hello71) wrote: > busybox echo fails if free sets errno, which madvise does on old > kernels. > --- > src/malloc/mallocng/free.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/src/malloc/mallocng/free.c b/src/malloc/mallocng/free.c > index 40745f97..82836815 100644 > --- a/src/malloc/mallocng/free.c > +++ b/src/malloc/mallocng/free.c > @@ -119,7 +119,13 @@ void free(void *p) > if (((uintptr_t)(start-1) ^ (uintptr_t)end) >= 2*PGSZ && g->last_idx) { > unsigned char *base = start + (-(uintptr_t)start & (PGSZ-1)); > size_t len = (end-base) & -PGSZ; > - if (len) madvise(base, len, MADV_FREE); > + if (len) { > + // madvise(..., MADV_FREE) returns -EINVAL on old kernels > + // POSIX.1-202x requires free() to not modify errno on success > + int e = errno; > + madvise(base, len, MADV_FREE); > + errno = e; > + } > } glue.h is already responsible for wiring up madvise appropriately (namespace-safe), so we could just change it to make a raw syscall instead of the function call to __madvise. This would be slightly less costly at runtime, but is kinda non-obvious to the reader (especially if the name is retained) and not as friendly to using mallocng standalone outside musl. > // atomic free without locking if this is neither first or last slot > @@ -139,5 +145,9 @@ void free(void *p) > wrlock(); > struct mapinfo mi = nontrivial_free(g, idx); > unlock(); > - if (mi.len) munmap(mi.base, mi.len); > + // POSIX.1-202x requires free() to not modify errno on success > + // munmap should succeed but no harm checking it again > + if (mi.len) > + if (munmap(mi.base, mi.len)) > + a_crash(); > } > -- > 2.30.0 This is utterly wrong and will crash correct programs. Unmapping memory can create 2 (temporarily 3) VMAs from one, thereby exceeding the VMA limit and failing. In this case you have to just accept the memory leak; you can't kill the valid program because the kernel is incapable of handling its request in a way that doesn't waste memory. You also can't do a raw syscall here, because munmap must wait for the vmlock. So some additional work to save/restore errno is needed, or else we need to expose a non-errno-using version of __munmap and use it. Rich