[RFC] Supporting malloc_usable_size

  Hello,

This is in context of this systemd issue:

https://github.com/systemd/systemd/issues/22801

through which I had discovered that systemd was (ab)using
malloc_usable_size to use spare space in an allocated object.  This was
discovered when _FORTIFY_SOURCE=3 flagged this as a buffer overflow,
since the compiler is unable to see that the space beyond the allocation
was safe to use.

At that time, I thought this use was to avoid reallocating[1] when
there's enough space in the usable size and that is not entirely false.
I had then proposed during my Cauldron talk (and in discussions with
some of you) that maybe providing a fast path for cases where the new
size was within the current chunk size was the solution to the problem
systemd was trying to solve with malloc_usable_size.

However I was wrong, it looks like the malloc_usable_size usage goes far
beyond simply optimizing allocation, it is used as a way to query sizes
of objects and hence eliminating the need to pass their corresponding
sizes.  This is probably suboptimal (given that there's a call into
libc.so at every point instead of what could have been just an
additional parameter alongside the object) but it's the design pattern
they've chosen.

I have an idea to support this abuse and at the same time, satisfy the
compiler by giving it a hint of the new size.  This could be done within
systemd by calling a dummy allocator (e.g. expand_to_usable) with the
result of malloc_usable_size(), thus telling the compiler that the
object has been expanded to the usable size, thus making accesses into
the spare space well defined.  I have a patch too (see below) to systemd
that demonstrates how this could be achieved, it's been tested with gcc
as well as clang and with _FORTIFY_SOURCE=3.

The larger consequence of this patch though is that we further support
the usage of malloc_usable_size for cases beyond diagnostics.  Do we
want to do that?  If we do, should we also then make clear what kind of
usage we support as a library, say, in the manual?  We don't have a
manual blurb for malloc_usable_size, so maybe this is a good opportunity
to do that.

Any other thoughts on malloc_usable_size?

Thanks,
Sid


[1] https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-level#the_gains_of_improved_security_coverage_outweigh_the_cost

---
 src/basic/alloc-util.c     |  5 +++++
 src/basic/alloc-util.h     | 16 ++++++++++++++--
 src/test/test-alloc-util.c |  5 +++--
 3 files changed, 22 insertions(+), 4 deletions(-)
  

Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
> At that time, I thought this use was to avoid reallocating[1] when

You can't do an optimal realloc if you don't know what the usable size
is, but once you know the optimal size, calling realloc() should be a
valid way of telling the compiler what you're doing.  We just need to
ensure that our realloc() does the sane thing in such cases and I see no
problem with users doing it.

> that maybe providing a fast path for cases where the new
> size was within the current chunk size was the solution to the problem
> systemd was trying to solve with malloc_usable_size.

I think a fast path check would be good for performance anyway, but
let's not call it a solution, because it won't change the functionality,
just the speed.

> it is used as a way to query sizes of objects and hence eliminating
> the need to pass their corresponding sizes.

Since malloc_usable_size is nonstandard and intended for debugging, this
is a bad programming practice (as noted in the man page).  As such, I
see no need to spend effort on it, either to block it or to optimize it.
Caveat programmer.  As long as the function returns the correct value, I
have no desire to worry about it further ;-)

> I have an idea to support this abuse and at the same time, satisfy the
> compiler by giving it a hint of the new size.

That's a lot of work to avoid a call to realloc().  And if they're
calling into libc to get a record size, they already don't care about
optimal performance.

> The larger consequence of this patch though is that we further support
> the usage of malloc_usable_size for cases beyond diagnostics.  Do we
> want to do that?

To what extend do we support it *at all*, other than "it returns the
right number at the time it was called" ?

> If we do, should we also then make clear what kind of usage we support
> as a library, say, in the manual?

Our man page already says "The main use of this function is for
debugging and introspection".  The BSD manual says "The
malloc_usable_size() function is not a mechanism for in-place realloc();
rather it is provided solely as a tool for introspection purposes."

So clarifying the manual to be more in line with BSD is probably OK but
anything further than that is IMHO an API change.

We may want to add a caveat that the returned value is only valid until
the next call to any malloc family function, though, although I don't
see any way our code could make the usable size of an allocated chunk
*smaller*.  I don't think we've ever guaranteed that the heap metadata
is constant.

Maybe as a lark we could make 1% of the calls return an obviously wrong
value?  ;-)
  

> On 2 Dec 2022, at 04:42, DJ Delorie via Libc-alpha <libc-alpha@sourceware.org> wrote:
> 
> [snip]

>> The larger consequence of this patch though is that we further support
>> the usage of malloc_usable_size for cases beyond diagnostics.  Do we
>> want to do that?
> 
> To what extend do we support it *at all*, other than "it returns the
> right number at the time it was called" ?

Right. It's still not clear to me if glibc is actually interested in supporting
the use case here. If it isn't, it should be stated clearly so it's clear
who is to blame when FORTIFY_SOURCE=3 complains.

(And it's also unclear to me that it should be supported, either.)

> 
>> If we do, should we also then make clear what kind of usage we support
>> as a library, say, in the manual?
> 
> Our man page already says "The main use of this function is for
> debugging and introspection".  The BSD manual says "The
> malloc_usable_size() function is not a mechanism for in-place realloc();
> rather it is provided solely as a tool for introspection purposes."
> 

My guess is the systemd maintainers currently rely on a specific interpretation
of "introspection" which may not align with glibc's.
  

Sam James <sam@gentoo.org> writes:
> Right. It's still not clear to me if glibc is actually interested in supporting
> the use case here. If it isn't, it should be stated clearly so it's clear
> who is to blame when FORTIFY_SOURCE=3 complains.

I don't think it's up to glibc to support a "use case" per se.  The API
does what is documented, no more, no less.  As long as we function
"correctly", the users can abuse that functionality all they want.  My
opinion is just that, when they do that, it's up to them to make sure
their abuse plays well with other tools, like gcc and FORTIFY_SOURCE=3.

Hence my focus on documentation.

We can document what the APIs do.

We can provide a tutorial that helps people understand how the APIs work
together in a "best practices" way.

We can list caveats that document whar be dragons.

Beyond that, caveat programmer.
  

On Nov 24 2022, Siddhesh Poyarekar wrote:

> This is in context of this systemd issue:
>
> https://github.com/systemd/systemd/issues/22801
>
> through which I had discovered that systemd was (ab)using
> malloc_usable_size to use spare space in an allocated object.  This was
> discovered when _FORTIFY_SOURCE=3 flagged this as a buffer overflow,
> since the compiler is unable to see that the space beyond the allocation
> was safe to use.

Which it isn't.  Nothing prevents malloc to hand out the extra space to
a different thread any time, so the size returned by malloc_usable_size
can get outdated instantly.
  

On 2022-12-02 07:03, Andreas Schwab wrote:
> On Nov 24 2022, Siddhesh Poyarekar wrote:
> 
>> This is in context of this systemd issue:
>>
>> https://github.com/systemd/systemd/issues/22801
>>
>> through which I had discovered that systemd was (ab)using
>> malloc_usable_size to use spare space in an allocated object.  This was
>> discovered when _FORTIFY_SOURCE=3 flagged this as a buffer overflow,
>> since the compiler is unable to see that the space beyond the allocation
>> was safe to use.
> 
> Which it isn't.  Nothing prevents malloc to hand out the extra space to
> a different thread any time, so the size returned by malloc_usable_size
> can get outdated instantly.

Not with the current malloc implementation I suppose but I get what you 
mean.  Florian had mentioned a similar caveat where a malloc 
implementation could coalesce adjacent free blocks with the end of an 
allocated block and change the value of malloc_usable_size at any 
arbitrary point in time too.

However the man page starts with "Although the excess bytes can be 
overwritten by the application without ill effects" and maybe that 
reassurance needs to be dropped.

Thanks,
Sid
  

On Dez 02 2022, Siddhesh Poyarekar wrote:

> However the man page starts with "Although the excess bytes can be
> overwritten by the application without ill effects" and maybe that
> reassurance needs to be dropped.

Or perhaps amended: "until the next call to malloc/realloc/free in any
thread".
  

On 2022-12-02 00:28, DJ Delorie wrote:
> Sam James <sam@gentoo.org> writes:
>> Right. It's still not clear to me if glibc is actually interested in supporting
>> the use case here. If it isn't, it should be stated clearly so it's clear
>> who is to blame when FORTIFY_SOURCE=3 complains.
> 
> I don't think it's up to glibc to support a "use case" per se.  The API
> does what is documented, no more, no less.  As long as we function
> "correctly", the users can abuse that functionality all they want.  My
> opinion is just that, when they do that, it's up to them to make sure
> their abuse plays well with other tools, like gcc and FORTIFY_SOURCE=3.
> 
> Hence my focus on documentation.
> 
> We can document what the APIs do.
> 
> We can provide a tutorial that helps people understand how the APIs work
> together in a "best practices" way.
> 
> We can list caveats that document whar be dragons.
> 
> Beyond that, caveat programmer.

Thanks, from your and Sam's comments, one thing I can be sure of is that 
I (as glibc maintainer) should not be the one suggesting hacks that add 
some measure of safety to this use since that then may get misconstrued 
as endorsement by the glibc project.  It has happened before:

https://github.com/systemd/systemd/issues/22801#issuecomment-1073962482

Besides, both Andreas and Florian pointed out ways in which such 
malloc_usable_size could be unsafe despite current definitions, so that 
is further reason to not support this use case.

On to the alternative question then; given that the interface has 
minimal utility, unnecessarily exposes internal implementation caveats 
and is prone to abuse, does it make sense to deprecate it?  If not, does 
it make sense to make the note in the man page stronger by, e.g. 
removing the "without ill effects" and discourage its use for anything 
other than diagnostics?

Thanks,
Sid
  

* Andreas Schwab:

> On Dez 02 2022, Siddhesh Poyarekar wrote:
>
>> However the man page starts with "Although the excess bytes can be
>> overwritten by the application without ill effects" and maybe that
>> reassurance needs to be dropped.
>
> Or perhaps amended: "until the next call to malloc/realloc/free in any
> thread".

The list will never be complete because glibc can call into the malloc
subsystem internally, or even spontaneously from an internal service
thread.

Thanks,
Florian
  

* Siddhesh Poyarekar:

> On 2022-12-02 07:03, Andreas Schwab wrote:
>> On Nov 24 2022, Siddhesh Poyarekar wrote:
>> 
>>> This is in context of this systemd issue:
>>>
>>> https://github.com/systemd/systemd/issues/22801
>>>
>>> through which I had discovered that systemd was (ab)using
>>> malloc_usable_size to use spare space in an allocated object.  This was
>>> discovered when _FORTIFY_SOURCE=3 flagged this as a buffer overflow,
>>> since the compiler is unable to see that the space beyond the allocation
>>> was safe to use.
>> Which it isn't.  Nothing prevents malloc to hand out the extra space
>> to
>> a different thread any time, so the size returned by malloc_usable_size
>> can get outdated instantly.
>
> Not with the current malloc implementation I suppose but I get what
> you mean.  Florian had mentioned a similar caveat where a malloc 
> implementation could coalesce adjacent free blocks with the end of an
> allocated block and change the value of malloc_usable_size at any 
> arbitrary point in time too.
>
> However the man page starts with "Although the excess bytes can be
> overwritten by the application without ill effects" and maybe that 
> reassurance needs to be dropped.

I think that is part of the interface contract.

During the life-time of an allocation, the observed return value can
only ever grow, never shrink.  That's how I read the interface
description (because it doesn't say the value is constant).

However, systemd has cases which seem incompatible even with that: The
code derives an array length from the malloc_usable_size return value.
It assumes that if it has initialized the array up to that length at one
point, all array elements are initialized.  But when the array length is
recomputed, new uninitialized elements may appear, so that's not really
correct.

Thanks,
Florian
  

Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
> On to the alternative question then; given that the interface has 
> minimal utility, unnecessarily exposes internal implementation caveats 
> and is prone to abuse, does it make sense to deprecate it?  If not, does 
> it make sense to make the note in the man page stronger by, e.g. 
> removing the "without ill effects" and discourage its use for anything 
> other than diagnostics?

I see no reason to deprecate this call, as long as it does what it's
documented to do.  Just because we think it's a bad API doesn't mean
some other developer won't find it immensely useful in some way we
haven't imagined.

Existing notes:

  The value returned by malloc_usable_size() may be greater than the
  requested size of the allocation because of alignment and minimum size
  constraints.  Although the excess bytes can be overwritten by the
  application without ill effects, this is not good programming
  practice: the number of excess bytes in an allocation depends on the
  underlying implementation.

Suggested text:?

  The value returned by malloc_usable_size() may be greater than the
  requested size of the allocation because of various internal
  implementation details, none of which the programmer should rely on.
  This function is intended to only be used for diagnostics and
  statistics; writing to the excess memory without first calling
  realloc() to resize the allocation is not supported.  The returned
  value is only valid at the time of the call; any other call to a
  malloc family API may invalidate it.

I think this is as harsh as we should go, and I'm not opposed to
removing any of the above if we think it oversteps the ABI bounds.
  

On 2022-12-02 14:16, DJ Delorie wrote:
>    The value returned by malloc_usable_size() may be greater than the
>    requested size of the allocation because of various internal
>    implementation details, none of which the programmer should rely on.
>    This function is intended to only be used for diagnostics and
>    statistics; writing to the excess memory without first calling
>    realloc() to resize the allocation is not supported.  The returned
>    value is only valid at the time of the call; any other call to a
>    malloc family API may invalidate it.

That sounds reasonable.  I wonder if the last statement could be taken 
as a guarantee of validity up to a certain point (i.e. another call to a 
malloc family API); should that be simply "The returned value is only 
valid at the time of the call."?

Thanks,
Sid
  

Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
> That sounds reasonable.  I wonder if the last statement could be taken 
> as a guarantee of validity up to a certain point (i.e. another call to a 
> malloc family API); should that be simply "The returned value is only 
> valid at the time of the call."?

Good point.  Agreed.
  

On 2022-12-02 7:39 AM, Florian Weimer via Libc-alpha wrote:
> * Andreas Schwab:
> 
>> On Dez 02 2022, Siddhesh Poyarekar wrote:
>>
>>> However the man page starts with "Although the excess bytes can be
>>> overwritten by the application without ill effects" and maybe that
>>> reassurance needs to be dropped.
>>
>> Or perhaps amended: "until the next call to malloc/realloc/free in any
>> thread".
> 
> The list will never be complete because glibc can call into the malloc
> subsystem internally

How about this then?  "The excess bytes, if any, are only guaranteed to 
exist until the next call to malloc/realloc/free in any thread.  Note 
that almost all C library functions are allowed to use malloc internally 
for scratch space, and the list of functions that do so may change 
without notice.  Only the functions documented as async-signal-safe are 
guaranteed not to use malloc internally."

> or even spontaneously from an internal service thread

We don't have any of those now, do we?  I'm inclined to say that we 
_shouldn't_ have any of those.

zw
  

On 2022-12-05 13:46, Zack Weinberg via Libc-alpha wrote:
> On 2022-12-02 7:39 AM, Florian Weimer via Libc-alpha wrote:
>> * Andreas Schwab:
>>
>>> On Dez 02 2022, Siddhesh Poyarekar wrote:
>>>
>>>> However the man page starts with "Although the excess bytes can be
>>>> overwritten by the application without ill effects" and maybe that
>>>> reassurance needs to be dropped.
>>>
>>> Or perhaps amended: "until the next call to malloc/realloc/free in any
>>> thread".
>>
>> The list will never be complete because glibc can call into the malloc
>> subsystem internally
> 
> How about this then?  "The excess bytes, if any, are only guaranteed to 
> exist until the next call to malloc/realloc/free in any thread.  Note 
> that almost all C library functions are allowed to use malloc internally 
> for scratch space, and the list of functions that do so may change 
> without notice.  Only the functions documented as async-signal-safe are 
> guaranteed not to use malloc internally."

DJ has also made a suggestion for this[1], which seems similar to what 
you have.  The only concern I have is the additional guarantee the text 
appears to provide (valid till the next allocator call); I'd personally 
prefer something that gives absolutely no guarantees.

Maybe one of you could volunteer to propose this to the man pages project?

Thanks,
Sid

[1] https://sourceware.org/pipermail/libc-alpha/2022-December/143701.html
  

* Zack Weinberg via Libc-alpha:

> On 2022-12-02 7:39 AM, Florian Weimer via Libc-alpha wrote:
>> * Andreas Schwab:
>> 
>>> On Dez 02 2022, Siddhesh Poyarekar wrote:
>>>
>>>> However the man page starts with "Although the excess bytes can be
>>>> overwritten by the application without ill effects" and maybe that
>>>> reassurance needs to be dropped.
>>>
>>> Or perhaps amended: "until the next call to malloc/realloc/free in any
>>> thread".
>> The list will never be complete because glibc can call into the
>> malloc
>> subsystem internally
>
> How about this then?  "The excess bytes, if any, are only guaranteed
> to exist until the next call to malloc/realloc/free in any thread.
> Note that almost all C library functions are allowed to use malloc
> internally for scratch space, and the list of functions that do so may
> change without notice.  Only the functions documented as
> async-signal-safe are guaranteed not to use malloc internally."

I think it's a backwards-incompatible change.  The existing manual page
documents the function as MT-Safe.

With this new policy, I don't think malloc_usable_size is useful for
anything at all, and we can just deprecate it (with a deprecation
warning message) and eventually remove it from linking (after
considering the impact on replacement malloc implementations, but I
don't think it will be problematic).

>> or even spontaneously from an internal service thread
>
> We don't have any of those now, do we?  I'm inclined to say that we
> _shouldn't_ have any of those.

mq_notify and timer_create do this, I think.

Thanks,
Florian
  

On 2022-12-05 15:35, Florian Weimer via Libc-alpha wrote:
> I think it's a backwards-incompatible change.  The existing manual page
> documents the function as MT-Safe.
> 
> With this new policy, I don't think malloc_usable_size is useful for
> anything at all, and we can just deprecate it (with a deprecation
> warning message) and eventually remove it from linking (after
> considering the impact on replacement malloc implementations, but I
> don't think it will be problematic).

We don't have consensus on deprecation unfortunately.  DJ is of the 
opinion that the limited use (knowing the usable size but not being able 
to do anything with it) may be useful enough for some programs.  With 
that position, making it clear that the extra space is never safely 
usable and then just leaving it at that is probably the least worst out.

Sid
  

* Siddhesh Poyarekar:

> On 2022-12-05 15:35, Florian Weimer via Libc-alpha wrote:
>> I think it's a backwards-incompatible change.  The existing manual page
>> documents the function as MT-Safe.
>> With this new policy, I don't think malloc_usable_size is useful for
>> anything at all, and we can just deprecate it (with a deprecation
>> warning message) and eventually remove it from linking (after
>> considering the impact on replacement malloc implementations, but I
>> don't think it will be problematic).
>
> We don't have consensus on deprecation unfortunately.  DJ is of the
> opinion that the limited use (knowing the usable size but not being
> able to do anything with it) may be useful enough for some programs.
> With that position, making it clear that the extra space is never
> safely usable and then just leaving it at that is probably the least
> worst out.

Maybe we could add a deprecation warning just for -D_FORTIFY_SOURCE=3?

DJ, what do you think?

Thanks,
Florian
  

On 2022-12-07 05:01, Florian Weimer wrote:
> * Siddhesh Poyarekar:
> 
>> On 2022-12-05 15:35, Florian Weimer via Libc-alpha wrote:
>>> I think it's a backwards-incompatible change.  The existing manual page
>>> documents the function as MT-Safe.
>>> With this new policy, I don't think malloc_usable_size is useful for
>>> anything at all, and we can just deprecate it (with a deprecation
>>> warning message) and eventually remove it from linking (after
>>> considering the impact on replacement malloc implementations, but I
>>> don't think it will be problematic).
>>
>> We don't have consensus on deprecation unfortunately.  DJ is of the
>> opinion that the limited use (knowing the usable size but not being
>> able to do anything with it) may be useful enough for some programs.
>> With that position, making it clear that the extra space is never
>> safely usable and then just leaving it at that is probably the least
>> worst out.
> 
> Maybe we could add a deprecation warning just for -D_FORTIFY_SOURCE=3?

I'm all for deprecation, at least partial if not full.

Thanks,
Sid
  

On 07/12/22 13:34, Siddhesh Poyarekar wrote:
> On 2022-12-07 05:01, Florian Weimer wrote:
>> * Siddhesh Poyarekar:
>>
>>> On 2022-12-05 15:35, Florian Weimer via Libc-alpha wrote:
>>>> I think it's a backwards-incompatible change.  The existing manual page
>>>> documents the function as MT-Safe.
>>>> With this new policy, I don't think malloc_usable_size is useful for
>>>> anything at all, and we can just deprecate it (with a deprecation
>>>> warning message) and eventually remove it from linking (after
>>>> considering the impact on replacement malloc implementations, but I
>>>> don't think it will be problematic).
>>>
>>> We don't have consensus on deprecation unfortunately.  DJ is of the
>>> opinion that the limited use (knowing the usable size but not being
>>> able to do anything with it) may be useful enough for some programs.
>>> With that position, making it clear that the extra space is never
>>> safely usable and then just leaving it at that is probably the least
>>> worst out.
>>
>> Maybe we could add a deprecation warning just for -D_FORTIFY_SOURCE=3?
> 
> I'm all for deprecation, at least partial if not full.
> 
> Thanks,
> Sid

I tend to agree with full deprecation, systemd MALLOC_SIZEOF_SAFE is an
example of what user needs to hack around with an ill defined interface and
_FORTIFY_SOURCE=3 warning should be clear that keep it is not a gain in long 
term (besides adding even more trouble for portability).

Another possibility would to make malloc_usable_size always return the malloc
size argument (although it would add some internal metadata costs).
  

> On 7 Dec 2022, at 16:54, Adhemerval Zanella Netto via Libc-alpha <libc-alpha@sourceware.org> wrote:
> [snip]

> I tend to agree with full deprecation, systemd MALLOC_SIZEOF_SAFE is an
> example of what user needs to hack around with an ill defined interface and
> _FORTIFY_SOURCE=3 warning should be clear that keep it is not a gain in long
> term (besides adding even more trouble for portability).
> 
> Another possibility would to make malloc_usable_size always return the malloc
> size argument (although it would add some internal metadata costs).

My position is already clear (full deprecation), but if someone does feel
it's useful fo them, they can always speak up over the lengthy
deprecation period anyway. It could always be reversed if necessary.
  

* Adhemerval Zanella Netto:

> Another possibility would to make malloc_usable_size always return the
> malloc size argument (although it would add some internal metadata
> costs).

We can probably implement it with little overhead, even on 32 bit, but a
size-class-based allocator will require more drastic changes.

Thanks,
Florian
  

Florian Weimer <fweimer@redhat.com> writes:
> DJ, what do you think?

The world seems set against me to get rid of this API, so don't let me
stop you.  I just don't see the point, especially as other packages use
it and will FTBFS when it's removed.

Also, BSD provides this API too.
  

On 2022-12-07 11:54, Adhemerval Zanella Netto wrote:
> I tend to agree with full deprecation, systemd MALLOC_SIZEOF_SAFE is an
> example of what user needs to hack around with an ill defined interface and
> _FORTIFY_SOURCE=3 warning should be clear that keep it is not a gain in long
> term (besides adding even more trouble for portability).
> 
> Another possibility would to make malloc_usable_size always return the malloc
> size argument (although it would add some internal metadata costs).
> 

Do we want to go down this path?  I was quite reluctant until I 
remembered that there's P0401R6 for C++ that would need this kind of 
information for support anyway.  The intent of that paper is different, 
it's to optimize for realloc and that would actually need us to endorse 
the chunk size as the actual size when we return it.  That would then 
disallow chunks from being consolidated, thus making usable size 
consistent through the lifetime of the object.

Basically, it looks like one way or another, there's value in making the 
return value of malloc_usable_size consistent and maybe even usable for 
writes if we are to support something like P0401R6 in future and 
instead, help the compiler see this usable size.

Thoughts?

Thanks,
Siddhesh

[1] 
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p0401r6.html#deallocate