Re: [RFC PATCH] malloc_usable_size.3: Warn about _FORTIFY_SOURCE interaction

Re: [RFC PATCH] malloc_usable_size.3: Warn about _FORTIFY_SOURCE interaction

[Siddhesh Poyarekar]

On 2023-04-04 01:52, Mingye Wang wrote:
> Hi all,
> 
> In (somewhat) recent discussions about _FORTIFY_SOURCE level 3, a
> common snag to hit seems to be abuse of malloc_usable_size(). The
> attached patch is my attempt at making the situation easier to sort
> through.
> 
> See siddhesh's comment on GitHub.[0] I wonder if the language needs to
> be stronger.
>    [0]: https://github.com/systemd/systemd/issues/22801#issuecomment-1343041481

For more context of my statement, please see this discussion:

https://sourceware.org/pipermail/libc-alpha/2022-November/143599.html

which continued into the next month:

https://sourceware.org/pipermail/libc-alpha/2022-December/143667.html

This amendment that DJ wrote is probably the most precise description of 
the current malloc_usage_size situation:

   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.

Thanks,
Sid

Re: [RFC PATCH] malloc_usable_size.3: Warn about _FORTIFY_SOURCE interaction

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]


Siddhesh Poyarekar <siddhesh@gotplt.org> writes:

> On 2023-04-04 01:52, Mingye Wang wrote:
>> Hi all,
>> In (somewhat) recent discussions about _FORTIFY_SOURCE level 3, a
>> common snag to hit seems to be abuse of malloc_usable_size(). The
>> attached patch is my attempt at making the situation easier to sort
>> through.
>> See siddhesh's comment on GitHub.[0] I wonder if the language needs
>> to
>> be stronger.
>>    [0]: https://github.com/systemd/systemd/issues/22801#issuecomment-1343041481
>
> For more context of my statement, please see this discussion:
>
> https://sourceware.org/pipermail/libc-alpha/2022-November/143599.html
>
> which continued into the next month:
>
> https://sourceware.org/pipermail/libc-alpha/2022-December/143667.html
>
> This amendment that DJ wrote is probably the most precise description
> of the current malloc_usage_size situation:
>
>   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.

Honestly, I thought we'd committed that. Oops.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 377 bytes --]

Re: [RFC PATCH] malloc_usable_size.3: Warn about _FORTIFY_SOURCE interaction

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 4261 bytes --]

Hi Siddhesh,

On 4/4/23 13:42, Siddhesh Poyarekar wrote:
> On 2023-04-04 01:52, Mingye Wang wrote:
>> Hi all,
>>
>> In (somewhat) recent discussions about _FORTIFY_SOURCE level 3, a
>> common snag to hit seems to be abuse of malloc_usable_size(). The
>> attached patch is my attempt at making the situation easier to sort
>> through.
>>
>> See siddhesh's comment on GitHub.[0] I wonder if the language needs to
>> be stronger.
>>    [0]: https://github.com/systemd/systemd/issues/22801#issuecomment-1343041481
> 
> For more context of my statement, please see this discussion:
> 
> https://sourceware.org/pipermail/libc-alpha/2022-November/143599.html
> 
> which continued into the next month:
> 
> https://sourceware.org/pipermail/libc-alpha/2022-December/143667.html

This might be useful to you: I happen to comaintain a code base that
uses malloc_usable_size(3).  I have no idea why it was added, and it
seems to be used in a test, but not in the actual program, which makes
me happy to not have to fix that :).  Maybe it is useful to you to check
that code to see why would some heavily-optimized code base want to use
it.  You may very well find that it was not really used for anything
useful; there's a lot of dead code which I haven't been able to remove
yet due to discrepancies.

Here are all the mentions I see to this API:

$ grep -rn malloc_usable_size
src/test/nxt_malloc_test.c:54:        nxt_malloc_usable_size(p[i], s);
src/nxt_malloc.h:37: * memory than is requested, so malloc_usable_size() allows to use all
src/nxt_malloc.h:52: * with small cutback and then to adjust size with malloc_usable_size().
src/nxt_malloc.h:53: * Glibc malloc_usable_size() is fast operation.
src/nxt_malloc.h:56:#define nxt_malloc_usable_size(p, size)                                       \
src/nxt_malloc.h:57:    size = malloc_usable_size(p)
src/nxt_malloc.h:77: * FreeBSD 7.0 malloc_usable_size() is fast for allocations, which
src/nxt_malloc.h:81:#define nxt_malloc_usable_size(p, size)                                       \
src/nxt_malloc.h:82:    size = malloc_usable_size(p)
src/nxt_malloc.h:101:#define nxt_malloc_usable_size(p, size)                                       \
src/nxt_malloc.h:108:#define nxt_malloc_usable_size(p, size)
src/nxt_unix.h:32:#include <malloc.h>                 /* malloc_usable_size(). */
src/nxt_unix.h:49:#include <malloc_np.h>              /* malloc_usable_size(). */
auto/malloc:53:# Linux malloc_usable_size().
auto/malloc:55:nxt_feature="Linux malloc_usable_size()"
auto/malloc:66:                      if (malloc_usable_size(p) < 4096)
auto/malloc:75:    # FreeBSD malloc_usable_size().
auto/malloc:77:    nxt_feature="FreeBSD malloc_usable_size()"
auto/malloc:89:                          if (malloc_usable_size(p) < 4096)

The only ones that may be interesting to you are the single use (the
commit that added the line says "Initial version.", so it won't help):

<https://github.com/nginx/unit/blob/c54331fa3d9597ba6bc85e7b7242981f00ed25c2/src/test/nxt_malloc_test.c#L54>

and the header where we define a wrapper macro, which contains several
comments about assumptions made about different libc implementations:

<https://github.com/nginx/unit/blob/c54331fa3d9597ba6bc85e7b7242981f00ed25c2/src/nxt_malloc.h#L35>

I hope that tells you something.  It doesn't tell me anything, but I'm
not used to fiddling with allocators.  :)

Cheers,
Alex


> 
> This amendment that DJ wrote is probably the most precise description of 
> the current malloc_usage_size situation:
> 
>    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.
> 
> Thanks,
> Sid

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] malloc_usable_size.3: Warn about _FORTIFY_SOURCE interaction

[Siddhesh Poyarekar]

On 2023-04-04 22:35, Alejandro Colomar wrote:
> This might be useful to you: I happen to comaintain a code base that
> uses malloc_usable_size(3).  I have no idea why it was added, and it
> seems to be used in a test, but not in the actual program, which makes
> me happy to not have to fix that :).  Maybe it is useful to you to check
> that code to see why would some heavily-optimized code base want to use
> it.  You may very well find that it was not really used for anything
> useful; there's a lot of dead code which I haven't been able to remove
> yet due to discrepancies.
> 
> Here are all the mentions I see to this API:
> 
> $ grep -rn malloc_usable_size
> src/test/nxt_malloc_test.c:54:        nxt_malloc_usable_size(p[i], s);
> src/nxt_malloc.h:37: * memory than is requested, so malloc_usable_size() allows to use all
> src/nxt_malloc.h:52: * with small cutback and then to adjust size with malloc_usable_size().
> src/nxt_malloc.h:53: * Glibc malloc_usable_size() is fast operation.
> src/nxt_malloc.h:56:#define nxt_malloc_usable_size(p, size)                                       \
> src/nxt_malloc.h:57:    size = malloc_usable_size(p)
> src/nxt_malloc.h:77: * FreeBSD 7.0 malloc_usable_size() is fast for allocations, which
> src/nxt_malloc.h:81:#define nxt_malloc_usable_size(p, size)                                       \
> src/nxt_malloc.h:82:    size = malloc_usable_size(p)
> src/nxt_malloc.h:101:#define nxt_malloc_usable_size(p, size)                                       \
> src/nxt_malloc.h:108:#define nxt_malloc_usable_size(p, size)
> src/nxt_unix.h:32:#include <malloc.h>                 /* malloc_usable_size(). */
> src/nxt_unix.h:49:#include <malloc_np.h>              /* malloc_usable_size(). */
> auto/malloc:53:# Linux malloc_usable_size().
> auto/malloc:55:nxt_feature="Linux malloc_usable_size()"
> auto/malloc:66:                      if (malloc_usable_size(p) < 4096)
> auto/malloc:75:    # FreeBSD malloc_usable_size().
> auto/malloc:77:    nxt_feature="FreeBSD malloc_usable_size()"
> auto/malloc:89:                          if (malloc_usable_size(p) < 4096)
> 
> The only ones that may be interesting to you are the single use (the
> commit that added the line says "Initial version.", so it won't help):
> 
> <https://github.com/nginx/unit/blob/c54331fa3d9597ba6bc85e7b7242981f00ed25c2/src/test/nxt_malloc_test.c#L54>
> 
> and the header where we define a wrapper macro, which contains several
> comments about assumptions made about different libc implementations:
> 
> <https://github.com/nginx/unit/blob/c54331fa3d9597ba6bc85e7b7242981f00ed25c2/src/nxt_malloc.h#L35>
> 
> I hope that tells you something.  It doesn't tell me anything, but I'm
> not used to fiddling with allocators.  :)

I'm sorry it doesn't, I can't tell from a quick peek what that test is 
trying to do :/

>> This amendment that DJ wrote is probably the most precise description of
>> the current malloc_usage_size situation:
>>
>>     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 should probably add that I'm trying to come up with something that 
will provide the functionality most people depend on malloc_usable_size 
for but with more defined semantics that doesn't simply leak internals 
like malloc_usable_size does.  However it's too early for me to promise 
anything and whatever solution that comes up will likely be ABI 
incompatible anyway.  So the above is the most precise description of 
the situation and whatever happens, we'll only end up adding to it, not 
replacing it.

Thanks,
Sid

Re: [RFC PATCH] malloc_usable_size.3: Warn about _FORTIFY_SOURCE interaction

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2629 bytes --]

Hi Wilco,

On 4/5/23 15:55, Wilco Dijkstra wrote:
> Hi Alejandro,
> 
>> and the header where we define a wrapper macro, which contains several
>> comments about assumptions made about different libc implementations:
>>
>> <https://github.com/nginx/unit/blob/c54331fa3d9597ba6bc85e7b7242981f00ed25c2/src/nxt_malloc.h#L35>
>>
>> I hope that tells you something.  It doesn't tell me anything, but I'm
>> not used to fiddling with allocators.  :)
> 
> This looks rather worrying

Agree.  I've seen all kinds of UB crimes in this codebase.  I'm trying to fix
all that, but it's hard to convince maintainers that lived in a cave for the
last few decades and don't know about C99, strict aliasing rules, or overflow,
to change certain code "if it worked so far" for some definition of "worked".
Of course, it is compiled with -O which is probably what has avoided hell
breaking loose so far (I'm honestly surprised that the performance of nginx is
so high compiling with -O, I must admit).

> - it seems to deliberately use a malloc size that is too
> small in the hope that the particular malloc implementation allocates a bit more
> and then use that extra space.

Yes, that's what it looked to me.  :/

> So every malloc call now needs extra checks to
> adjust the size and another call to malloc_usable_size which then needs to be
> checked to be larger than the original requested size...
> 
> So basically they are trying to save 32 bytes in blocks larger than 128KB
> (a whopping 0.024%!!!) by adding ~64 bytes of extra code per malloc call plus
> lots of extra executed instructions...

I think it was more like avoiding an entire page just for those 32 bytes of
bookkeeping.  So if the bookkeeping would trigger the allocation of a new
page, that is avoided.  It seems to be saving (4K - 32) bytes for blocks
of >128K, so <3% of the size; still a low number, which I wouldn't trade for
extra code and CPU overhead.  Luckily, it seems these macros are not really
being called :)

> 
> This kind of stupidity convinces me even more that we need to obsolete
> malloc_usable_size - people clearly cannot use it properly or avoid
> hardcoding internal implementation details which could change at any time.

I would say that not all of us are like that.  There's still programmers
who respect nasal demons.  However, I have never had a reason to call such
a function, so I'm not going to be the one that asks you to not remove it.

> 
> Cheers,
> Wilco

Cheers,
Alex

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] malloc_usable_size.3: Warn about _FORTIFY_SOURCE interaction

[Wilco Dijkstra]

Hi Alejandro,

> and the header where we define a wrapper macro, which contains several
> comments about assumptions made about different libc implementations:
> 
> <https://github.com/nginx/unit/blob/c54331fa3d9597ba6bc85e7b7242981f00ed25c2/src/nxt_malloc.h#L35>
> 
> I hope that tells you something.  It doesn't tell me anything, but I'm
> not used to fiddling with allocators.  :)

This looks rather worrying - it seems to deliberately use a malloc size that is too
small in the hope that the particular malloc implementation allocates a bit more
and then use that extra space. So every malloc call now needs extra checks to
adjust the size and another call to malloc_usable_size which then needs to be
checked to be larger than the original requested size...

So basically they are trying to save 32 bytes in blocks larger than 128KB
(a whopping 0.024%!!!) by adding ~64 bytes of extra code per malloc call plus
lots of extra executed instructions...

This kind of stupidity convinces me even more that we need to obsolete
malloc_usable_size - people clearly cannot use it properly or avoid
hardcoding internal implementation details which could change at any time.

Cheers,
Wilco