[asterisk-dev] [Code Review]: Fix memory leak in error paths for action_originate().

[Matt Jordan]

> On Jan. 25, 2012, 10:58 a.m., Matt Jordan wrote:
> > /branches/1.8/main/manager.c, line 3655
> > <https://reviewboard.asterisk.org/r/1690/diff/1/?file=23605#file23605line3655>
> >
> >     I'm not sure I agree with this change.  For read purposes, string fields are treated as const char * const - for write purposes, you have to use the string field API.  The data field ends up being passed as a void * to ast_request - and its up to the individual channel drivers to interpret them.
> >     
> >     A channel driver may assume that they can cast this to a char and modify the internals of the string field, going outside of the normal string field API calls.
> 
> rmudgett wrote:
>     Channel drivers should not do that anyway precisely because they cannot make an assumption about the buffer they are dealing with.
>     
>     I have not seen a channel driver or most other parsing functions in Asterisk that did not first copy the string into a local buffer for dissecting.

While I haven't looked through all of them, chan_phone assigns data to a char*:
  char *name = data;

Later on, chan_phone does the following:

  size_t length = strlen(p->dev + 5);
    if (strncmp(name, p->dev + 5, length) == 0 &&
       !isalnum(name[length])) {

p->dev is a char buffer of length 256.  Your string fields are initialized to a length of 252.  So, in this case, if someone originated through AMI and created a channel of this type, you'd end up looking into ... something in the string field pool, I'm not sure where or what.

Granted, chan_phone shouldn't be doing any of this.  Its making assumptions it shouldn't be making.  However, you're still stripping off the const'ness and assuming that the channel techs will do the right thing and treat it as immutable data.  Since it isn't documented anywhere that they need to treat the void * as immutable, it's not fair to assume that they will.


- Matt


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


On Jan. 25, 2012, 10:43 a.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1690/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2012, 10:43 a.m.)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Summary
> -------
> 
> * Fix memory leak of vars in error paths for action_originate().
> 
> * Moved struct fast_originate_helper tech and data members to stringfields.
> 
> * Simplified ActionID header handling for fast_originate().
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/manager.c 352510 
> 
> Diff: https://reviewboard.asterisk.org/r/1690/diff
> 
> 
> Testing
> -------
> 
> It compiles. :)
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120125/3c21ed96/attachment.htm>