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

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

[Mingye Wang]

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

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

Best,
Mingye Wang (Artoria2e5)

[-- Attachment #2: 0001-malloc_usable_size.3-Warn-about-_FORTIFY_SOURCE-inte.patch --]
[-- Type: application/octet-stream, Size: 1122 bytes --]

From f061522764ec417e80622db557853c2d7493bbb7 Mon Sep 17 00:00:00 2001
From: Mingye Wang <arthur200126@gmail.com>
Date: Tue, 4 Apr 2023 13:43:39 +0800
Subject: [PATCH] malloc_usable_size.3: Warn about _FORTIFY_SOURCE interaction

Abuse of malloc_usable_size() is common enough to snap up Redhat's
trials of -D_FORTIFY_SOURCE=3.  Warn against this to ease debugging.

Signed-Off-by: Mingye Wang <arthur200126@gmail.com>
---
 man3/malloc_usable_size.3 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/man3/malloc_usable_size.3 b/man3/malloc_usable_size.3
index 754b255de..1361e6f1e 100644
--- a/man3/malloc_usable_size.3
+++ b/man3/malloc_usable_size.3
@@ -62,5 +62,14 @@ the number of excess bytes in an allocation depends on
 the underlying implementation.
 .PP
 The main use of this function is for debugging and introspection.
+.PP
+.BR Warning :
+Some programs abuse
+.BR malloc_usable_size ()
+to reduce the number of calls to
+.BR realloc (3).
+Such use will confuse
+.BR _FORTIFY_SOURCE
+level 3, as it only keeps track of the original requested size.
 .SH SEE ALSO
 .BR malloc (3)
-- 
2.40.0.windows.1

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: 2520 bytes --]

Hi Mingye,

On 4/4/23 07: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
> 
> Best,
> Mingye Wang (Artoria2e5)


> From f061522764ec417e80622db557853c2d7493bbb7 Mon Sep 17 00:00:00 2001
> From: Mingye Wang <arthur200126@gmail.com>
> Date: Tue, 4 Apr 2023 13:43:39 +0800
> Subject: [PATCH] malloc_usable_size.3: Warn about _FORTIFY_SOURCE interaction
> 
> Abuse of malloc_usable_size() is common enough to snap up Redhat's
> trials of -D_FORTIFY_SOURCE=3.  Warn against this to ease debugging.
> 
> Signed-Off-by: Mingye Wang <arthur200126@gmail.com>
> ---
>  man3/malloc_usable_size.3 | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/man3/malloc_usable_size.3 b/man3/malloc_usable_size.3
> index 754b255de..1361e6f1e 100644
> --- a/man3/malloc_usable_size.3
> +++ b/man3/malloc_usable_size.3
> @@ -62,5 +62,14 @@ the number of excess bytes in an allocation depends on
>  the underlying implementation.
>  .PP
>  The main use of this function is for debugging and introspection.
> +.PP
> +.BR Warning :
> +Some programs abuse
> +.BR malloc_usable_size ()
> +to reduce the number of calls to
> +.BR realloc (3).
> +Such use will confuse
> +.BR _FORTIFY_SOURCE
> +level 3, as it only keeps track of the original requested size.

This is much milder than what I read in the linked discussions.
I would go ahead with something much stronger as suggested there,
or did you change your mind?

How about using DJ's suggestion in a CAVEATS section (I cut the
clause in the paragraph)?

    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.

Cheers,
Alex

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

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