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-le...@nginx.org
