* [musl] [PATCH] don't set errno in free [not found] <20210121140240.83405-1-alex_y_xu.ref@yahoo.ca> @ 2021-01-21 14:02 ` Alex Xu (Hello71) 2021-01-21 15:50 ` Natanael Copa 2021-01-21 16:27 ` Rich Felker 0 siblings, 2 replies; 6+ messages in thread From: Alex Xu (Hello71) @ 2021-01-21 14:02 UTC (permalink / raw) To: musl; +Cc: Alex Xu (Hello71) 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; + } } // 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] don't set errno in free 2021-01-21 14:02 ` [musl] [PATCH] don't set errno in free Alex Xu (Hello71) @ 2021-01-21 15:50 ` Natanael Copa 2021-01-21 16:18 ` Rich Felker 2021-01-21 16:27 ` Rich Felker 1 sibling, 1 reply; 6+ messages in thread From: Natanael Copa @ 2021-01-21 15:50 UTC (permalink / raw) To: Alex Xu (Hello71); +Cc: musl On Thu, 21 Jan 2021 09:02:40 -0500 "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca> 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; > + } > } I think we should save the errno early and make sure its restored on exit of the function. you should also include <errno.h>. I suggest something like: diff --git a/src/malloc/mallocng/free.c b/src/malloc/mallocng/free.c index 40745f97..77bed88b 100644 --- a/src/malloc/mallocng/free.c +++ b/src/malloc/mallocng/free.c @@ -1,6 +1,7 @@ #define _BSD_SOURCE #include <stdlib.h> #include <sys/mman.h> +#include <errno.h> #include "meta.h" @@ -102,6 +103,7 @@ void free(void *p) { if (!p) return; + int orig_errno = errno; struct meta *g = get_meta(p); int idx = get_slot_index(p); size_t stride = get_stride(g); @@ -133,11 +135,13 @@ void free(void *p) g->freed_mask = freed+self; else if (a_cas(&g->freed_mask, freed, freed+self)!=freed) continue; - return; + goto out; } wrlock(); struct mapinfo mi = nontrivial_free(g, idx); unlock(); if (mi.len) munmap(mi.base, mi.len); +out: + errno = orig_errno; } (looks like there are used names like errno_save, and old_errno in the code as well) > > // 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(); > } This should go into separate commit. -nc ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] don't set errno in free 2021-01-21 15:50 ` Natanael Copa @ 2021-01-21 16:18 ` Rich Felker 2021-01-21 16:20 ` Florian Weimer 2021-01-21 16:31 ` Natanael Copa 0 siblings, 2 replies; 6+ messages in thread From: Rich Felker @ 2021-01-21 16:18 UTC (permalink / raw) To: Natanael Copa; +Cc: Alex Xu (Hello71), musl On Thu, Jan 21, 2021 at 04:50:00PM +0100, Natanael Copa wrote: > On Thu, 21 Jan 2021 09:02:40 -0500 > "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca> 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; > > + } > > } > > I think we should save the errno early and make sure its restored on > exit of the function. you should also include <errno.h>. I suggest > something like: > > diff --git a/src/malloc/mallocng/free.c b/src/malloc/mallocng/free.c > index 40745f97..77bed88b 100644 > --- a/src/malloc/mallocng/free.c > +++ b/src/malloc/mallocng/free.c > @@ -1,6 +1,7 @@ > #define _BSD_SOURCE > #include <stdlib.h> > #include <sys/mman.h> > +#include <errno.h> > > #include "meta.h" > > @@ -102,6 +103,7 @@ void free(void *p) > { > if (!p) return; > > + int orig_errno = errno; This is much costlier. It puts the TLS access (faulting and emulating on old MIPS) in the path that runs on every call. Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] don't set errno in free 2021-01-21 16:18 ` Rich Felker @ 2021-01-21 16:20 ` Florian Weimer 2021-01-21 16:31 ` Natanael Copa 1 sibling, 0 replies; 6+ messages in thread From: Florian Weimer @ 2021-01-21 16:20 UTC (permalink / raw) To: Rich Felker; +Cc: Natanael Copa, musl, Alex Xu (Hello71) * Rich Felker: > This is much costlier. It puts the TLS access (faulting and emulating > on old MIPS) in the path that runs on every call. It's also a significant hit on certain modern AArch64 variants, which is a bit sad. Thanks, Florian -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] don't set errno in free 2021-01-21 16:18 ` Rich Felker 2021-01-21 16:20 ` Florian Weimer @ 2021-01-21 16:31 ` Natanael Copa 1 sibling, 0 replies; 6+ messages in thread From: Natanael Copa @ 2021-01-21 16:31 UTC (permalink / raw) To: Rich Felker; +Cc: musl, Alex Xu (Hello71) On Thu, 21 Jan 2021 11:18:08 -0500 Rich Felker <dalias@libc.org> wrote: > On Thu, Jan 21, 2021 at 04:50:00PM +0100, Natanael Copa wrote: > > On Thu, 21 Jan 2021 09:02:40 -0500 > > "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca> 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; > > > + } > > > } > > > > I think we should save the errno early and make sure its restored on > > exit of the function. you should also include <errno.h>. I suggest > > something like: > > > > diff --git a/src/malloc/mallocng/free.c b/src/malloc/mallocng/free.c > > index 40745f97..77bed88b 100644 > > --- a/src/malloc/mallocng/free.c > > +++ b/src/malloc/mallocng/free.c > > @@ -1,6 +1,7 @@ > > #define _BSD_SOURCE > > #include <stdlib.h> > > #include <sys/mman.h> > > +#include <errno.h> > > > > #include "meta.h" > > > > @@ -102,6 +103,7 @@ void free(void *p) > > { > > if (!p) return; > > > > + int orig_errno = errno; > > This is much costlier. It puts the TLS access (faulting and emulating > on old MIPS) in the path that runs on every call. I didn't think about that. The original suggestion is better then. Thanks! -nc > > Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] don't set errno in free 2021-01-21 14:02 ` [musl] [PATCH] don't set errno in free Alex Xu (Hello71) 2021-01-21 15:50 ` Natanael Copa @ 2021-01-21 16:27 ` Rich Felker 1 sibling, 0 replies; 6+ messages in thread From: Rich Felker @ 2021-01-21 16:27 UTC (permalink / raw) To: Alex Xu (Hello71); +Cc: musl 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-21 16:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20210121140240.83405-1-alex_y_xu.ref@yahoo.ca> 2021-01-21 14:02 ` [musl] [PATCH] don't set errno in free Alex Xu (Hello71) 2021-01-21 15:50 ` Natanael Copa 2021-01-21 16:18 ` Rich Felker 2021-01-21 16:20 ` Florian Weimer 2021-01-21 16:31 ` Natanael Copa 2021-01-21 16:27 ` Rich Felker
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).