fix accidental corrdump

[Maxim Dounin]

Hello!

On Thu, Sep 29, 2022 at 04:37:24PM -0400, Frank Swasey wrote:

> This is getting quite tiresome.  You are both stuck in your point of view
> and refusing to hear what the other one is saying.
> 
> Maxim - you keep repeating " l->alloc is not used after free(). "  Clearly,
> that is not true if setting it to NULL prevents the segfault.  What is true
> is that NGINX core code does not use it.  As a defensive coding technique,
> I agree with zjd that setting the pointer you just freed to NULL to
> indicate to any other code that is checking it is the proper action.  The
> only other thing that zjd can do is to set the pointer to NULL in their own
> code after calling the reset function if you are adamant that such
> defensive measures cannot be put into the NGINX core code.  Any future
> programmers that write modules like zjd has done that test a pointer for
> being NULL and use it if it has a non-NULL value, will trip over the same
> problem, and you can have this argument all over again.

The particular code is internal to nginx core, and the l->alloc is 
never used after free: this is something which can be easily 
seen within the ngx_reset_pool() function, which is about 20 
lines by itself.  That is, setting l->alloc to NULL is not a 
defensive coding technique by any means, and that's what I've 
tried to explain.

If setting l->alloc to NULL prevents the segfault, it is 
accidental, and likely indicate that zjd's module is using 
uninitialized and/or freed memory somewhere.  Trying to mitigate 
such bugs by setting arbitrary pointers to NULL is not going to 
fix these bugs.  Rather, this will make them harder to find and 
fix.  Instead, actions should be taken to make segfaults due to 
these bugs more likely, so it would be easier to find and fix 
them.

I've provided a few pointers on how to get segfaults due to such 
bugs more likely, hopefully it'll help zjd to find and fix bugs in 
his code.  In this particular case, I suspect that Address 
Sanitizer combined with NGX_DEBUG_PALLOC would be enough to 
immediately identify the bug.

What can be an improvement here is to introduce junk filling in 
the pool allocator code, for both new allocations and all blocks 
freed by ngx_reset_pool(), similarly to ngx_slab_junk() as used in 
the slab allocator.  I think that NGX_DEBUG_PALLOC would be more 
than enough in this particular case though, as this changes all 
pool allocations to use system allocator, and therefore makes it 
possible to configure junk filling and malloc checking on the OS 
level.

Sorry if this discussion bothers you.  It would be more 
appropriate in the nginx-devel@ mailing list, but the patch was 
posted here and it's probably too late to move the discussion 
anyway.

-- 
Maxim Dounin
http://mdounin.ru/