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/
_______________________________________________
nginx mailing list -- nginx@nginx.org
To unsubscribe send an email to nginx-leave@nginx.org