[asterisk-dev] [Code Review] MALLOC_HOLD for debugging memory

[Mark Michelson]

> On 2009-07-13 15:51:48, Mark Michelson wrote:
> > I question the usefulness of this change mainly because you can't see where in the code the illegal write to memory occurred. This also does not allow for the detection of illegal reads of freed memory.
> > 
> > Another potential problem (although this would never occur if Asterisk were perfect) is that the method used here is not thread-safe. You iterate over each byte of memory and see if the memory is non-NULL. The problem is that a section you already read could be written to after you pass over it. In order for this to work correctly, every write to memory would have to grab the reglock. That simply is not feasible.
> 
> Mark Michelson wrote:
>     Another problem is that this change masks any double frees which occur. A nice upside to this is that with a small bit of modification, you can actually detect and report double frees pretty easily with this code change.
>     
>     (I would have mentioned this earlier, but review board was adamantly refusing to let me edit my review further)
> 
> Tilghman Lesher wrote:
>     1) This is already threadsafe, as reglock is held during the entire time that the memory check is occurring.
>     2) Memory should not be written to after the memory is zeroed, because as far as the process is concerned, the memory is technically already freed (though not yet released back into the pool).  Therefore, any memory written to after the memory is zeroed is a possible memory corruption issue.
>     3) I understand that we cannot see what code wrote to the memory, but finding out where the memory was originally allocated is generally sufficient to track down that error.  This isn't Valgrind, after all, but merely a tool to help us when Valgrind cannot be deployed due to production concerns.
>     4) This does NOT mask double-frees.  We already check for that condition, and that check is not eliminated here.

Yes, the code is thread-safe with regards to allocations and frees, but it is not thread-safe if a thread is freeing a region and another thread is writing to an already-freed region which has been already-freed for 120 seconds or more. The thread freeing the region holds the reglock, but the thread writing to the already-freed region does even attempt to grab the reglock before writing to the already-freed memory. Now, obviously, the thread doing the writing should not even be trying to write to freed memory, but inevitably this happens. It would require some rather precise timing to occur, I'll admit, but it is possible.

If you ask me, seeing where the actual write occurred is just as important as seeing where the memory was originally freed. I don't see how you're supposed to be able to find the problematic area in the code if you don't see where the fault occurred.

You're going to have to show me how this doesn't mask double frees. I know the original code will print a "Freeing unused memory" message if a double free occurs. The problem here is that since the region is not removed from the list, you can free the same region twice and find it in the list of allocated regions both times. Therefore the "Freeing unused memory" message will not print. It is still possible to catch double frees here, but it would require the two occurrences to be at least 120 seconds apart and have at least one other region to be freed in the interval. In fact, taking a closer look now, I see that the "freeing unused memory" message cannot even print in MALLOC_HOLD mode since it is #ifdef'd out.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/309/#review966
-----------------------------------------------------------


On 2009-07-13 16:49:03, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/309/
> -----------------------------------------------------------
> 
> (Updated 2009-07-13 16:49:03)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> MALLOC_HOLD is an optional compile-time definition used to detect memory corruption.  It does this in a similar way to valgrind, although it does not slow down Asterisk appreciably (although the memory will grow somewhat larger than normal).
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/build_tools/cflags.xml 206144 
>   /branches/1.4/main/astmm.c 206144 
> 
> Diff: https://reviewboard.asterisk.org/r/309/diff
> 
> 
> Testing
> -------
> 
> Extensive testing with an idle system.
> 
> 
> Thanks,
> 
> Tilghman
> 
>