Message ID | fad9eac5-622d-b317-70da-311f6cac3557@gmail.com |
---|---|
State | New |
Series | "Fix crash in malloc_usable_size() if nullptr" |
Headers | show |
diff --git a/src/malloc/mallocng/malloc_usable_size.c b/src/malloc/mallocng/malloc_usable_size.c index a440a4ea..ce6a960c 100644 --- a/src/malloc/mallocng/malloc_usable_size.c +++ b/src/malloc/mallocng/malloc_usable_size.c @@ -3,6 +3,7 @@ size_t malloc_usable_size(void *p) { + if (!p) return 0; struct meta *g = get_meta(p); int idx = get_slot_index(p); size_t stride = get_stride(g);
On Wed, Nov 25, 2020 at 02:53:16AM -0500, Dominic Chen wrote: > Please CC me on replies. > > According to the manual for malloc_usable_size(), zero should be > returned if the input pointer is NULL. Currently, this is not > checked, which can result in SIGSEGV at runtime. > > Thanks, > > Dominic > > diff --git a/src/malloc/mallocng/malloc_usable_size.c b/src/malloc/mallocng/malloc_usable_size.c > index a440a4ea..ce6a960c 100644 > --- a/src/malloc/mallocng/malloc_usable_size.c > +++ b/src/malloc/mallocng/malloc_usable_size.c > @@ -3,6 +3,7 @@ > > size_t malloc_usable_size(void *p) > { > + if (!p) return 0; > struct meta *g = get_meta(p); > int idx = get_slot_index(p); > size_t stride = get_stride(g); Thanks. I wasn't aware of this. I did some research to see if this is actually documented as supported, since the Linux man pages aren't normative but just descriptive, and sometimes document things that aren't actually contracts. It seems glibc doesn't even document the existence of this function at all though. FreeBSD documents it but without any special handling of null pointers. But Solaris documents the same behavior you described. So it seems this is at least not entirely glibc-specific. Do you know if there are other implementations that do the same? Rich
On 11/25/2020 5:53 PM, Rich Felker wrote: > Thanks. I wasn't aware of this. I did some research to see if this is > actually documented as supported, since the Linux man pages aren't > normative but just descriptive, and sometimes document things that > aren't actually contracts. It seems glibc doesn't even document the > existence of this function at all though. > > FreeBSD documents it but without any special handling of null > pointers. But Solaris documents the same behavior you described. So it > seems this is at least not entirely glibc-specific. Do you know if > there are other implementations that do the same? Unfortunately, my understanding is just that this functionality (looking up heap allocation sizes) is platform-specific and not standardized. UNIX platforms seem to provide malloc_usable_size() (although it's not in SUSv4), whereas Mac provides malloc_size(), and Windows provides _msize(). Of other platforms, only the Windows documentation explicitly mentions null checks, but it calls a platform-specific invalid parameter handler instead of returning zero. On UNIX platforms, although documentation does not consistently describe returning zero for nullptr, I believe most do actually implement it. I took a look at the source for dlmalloc, glibc, jemalloc, and scudo, and can confirm that all four do so. Thanks, Dominic
On Wed, Nov 25, 2020 at 06:41:14PM -0500, Dominic Chen wrote: > On 11/25/2020 5:53 PM, Rich Felker wrote: > >Thanks. I wasn't aware of this. I did some research to see if this is > >actually documented as supported, since the Linux man pages aren't > >normative but just descriptive, and sometimes document things that > >aren't actually contracts. It seems glibc doesn't even document the > >existence of this function at all though. > > > >FreeBSD documents it but without any special handling of null > >pointers. But Solaris documents the same behavior you described. So it > >seems this is at least not entirely glibc-specific. Do you know if > >there are other implementations that do the same? > > Unfortunately, my understanding is just that this functionality > (looking up heap allocation sizes) is platform-specific and not > standardized. UNIX platforms seem to provide malloc_usable_size() > (although it's not in SUSv4), whereas Mac provides malloc_size(), > and Windows provides _msize(). Of other platforms, only the Windows > documentation explicitly mentions null checks, but it calls a > platform-specific invalid parameter handler instead of returning > zero. > > On UNIX platforms, although documentation does not consistently > describe returning zero for nullptr, I believe most do actually > implement it. I took a look at the source for dlmalloc, glibc, > jemalloc, and scudo, and can confirm that all four do so. Thanks for the followup. When implementing nonstandard extensions, we generally try to follow existing practice for what applications can "portably" expect from them. Here it sounds like accepting null is widespread, so I think it makes sense to apply your patch. Rich
On Wed, Nov 25, 2020 at 07:38:07PM -0500, Rich Felker wrote: > On Wed, Nov 25, 2020 at 06:41:14PM -0500, Dominic Chen wrote: > > On 11/25/2020 5:53 PM, Rich Felker wrote: > > >Thanks. I wasn't aware of this. I did some research to see if this is > > >actually documented as supported, since the Linux man pages aren't > > >normative but just descriptive, and sometimes document things that > > >aren't actually contracts. It seems glibc doesn't even document the > > >existence of this function at all though. > > > > > >FreeBSD documents it but without any special handling of null > > >pointers. But Solaris documents the same behavior you described. So it > > >seems this is at least not entirely glibc-specific. Do you know if > > >there are other implementations that do the same? > > > > Unfortunately, my understanding is just that this functionality > > (looking up heap allocation sizes) is platform-specific and not > > standardized. UNIX platforms seem to provide malloc_usable_size() > > (although it's not in SUSv4), whereas Mac provides malloc_size(), > > and Windows provides _msize(). Of other platforms, only the Windows > > documentation explicitly mentions null checks, but it calls a > > platform-specific invalid parameter handler instead of returning > > zero. > > > > On UNIX platforms, although documentation does not consistently > > describe returning zero for nullptr, I believe most do actually > > implement it. I took a look at the source for dlmalloc, glibc, > > jemalloc, and scudo, and can confirm that all four do so. > > Thanks for the followup. When implementing nonstandard extensions, we > generally try to follow existing practice for what applications can > "portably" expect from them. Here it sounds like accepting null is > widespread, so I think it makes sense to apply your patch. OK, this was already fixed once in commit d1507646975cbf6c3e511ba07b193f27f032d108 but regressed in mallocng so it's definitely appropriate to fix here. I found this when attempting to add oldmalloc support to your patch. I'm updating the commit message to reflect this and merging now. Thanks! Rich