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

[Alejandro Colomar <alx.manpages@gmail.com>]

[-- 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 --]