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

[Tilghman Lesher]

> 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)

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.


> On 2009-07-13 15:51:48, Mark Michelson wrote:
> > /branches/1.4/main/astmm.c, line 210
> > <https://reviewboard.asterisk.org/r/309/diff/1/?file=5925#file5925line210>
> >
> >     This declaration shadows the previous declaration of 'ptr' in this function. This should be changed to have a different name for clarity.
> >     
> >     Also, since all you're doing is comparing this value to 0 in the for loop below, it makes more sense for this to be a long pointer. That would require fewer iterations in the for loop. I'm not sure, however if the allocations are all multiples of a word, so this may require testing to be sure that no false positives are reported.

The multiples-of-a-word was my primary concern for doing it this way.  Also, on architectures where memory alignment matters, changing this to a long could have potential coredump implications.


> On 2009-07-13 15:51:48, Mark Michelson wrote:
> > /branches/1.4/main/astmm.c, lines 220-239
> > <https://reviewboard.asterisk.org/r/309/diff/1/?file=5925#file5925line220>
> >
> >     If you moved all the backtrace-printing logic to a separate function, you would not require as many preprocessor blocks as you do now.

The backtrace logic is actually disabled currently, because it crashes, and I haven't gone back and determined how to fix it.


> On 2009-07-13 15:51:48, Mark Michelson wrote:
> > /branches/1.4/main/astmm.c, lines 584-589
> > <https://reviewboard.asterisk.org/r/309/diff/1/?file=5925#file5925line584>
> >
> >     These changes appear to be unrelated to the rest of the code and should be included in a different commit.

No, they're related in that it was helpful to see the actual time that a fault occurred and not just the unixtime.


- Tilghman


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


On 2009-07-13 14:42:42, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/309/
> -----------------------------------------------------------
> 
> (Updated 2009-07-13 14:42:42)
> 
> 
> 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
> 
>