Fix crash in malloc_usable_size() if nullptr

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

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