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

[Mark Michelson]

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


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.


/branches/1.4/main/astmm.c
<https://reviewboard.asterisk.org/r/309/#comment2314>

    You should break from the for loop when this condition is true.



/branches/1.4/main/astmm.c
<https://reviewboard.asterisk.org/r/309/#comment2320>

    If you reverse the if and else conditions here, you can reduce the indentation level of the larger block by one.



/branches/1.4/main/astmm.c
<https://reviewboard.asterisk.org/r/309/#comment2311>

    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.



/branches/1.4/main/astmm.c
<https://reviewboard.asterisk.org/r/309/#comment2312>

    If you moved all the backtrace-printing logic to a separate function, you would not require as many preprocessor blocks as you do now.



/branches/1.4/main/astmm.c
<https://reviewboard.asterisk.org/r/309/#comment2317>

    These changes appear to be unrelated to the rest of the code and should be included in a different commit.


- Mark


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