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

[Mark Michelson]

> 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.
> 
> Matt Jordan wrote:
>     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 Jordan wrote:
>     FYI: chan_sip also just assigns the void * to a char *.  Although in that particular case, it shouldn't cause any problems, as it doesn't attempt to modify the contents of the char *, or index to some locally known location.
>     
>
> 
> rmudgett wrote:
>     That section of code in chan_phone is not changing the contents of data, nor is it accessing beyond the length of data.  The initial size of the string field memory pool is irrelevant here.  The string fields here are initialized with an allocated string memory pool with *initial* size of 252 and since it is allocated memory it can grow as needed.
>     
>     I have looked at chan_dahdi/sig_analog/sig_ss7/sig_pri, chan_iax2, chan_misdn, and chan_sip.  These channel drivers all copy the data string before parsing it.
>     
>     Stripping off const'ness is not automatically a bad thing.
>     
>     A code improvement (for another patch) would be to make more of the tech callbacks specify const strings.

I was going to suggest that the "correct" change is to make the data in ast_request a const char * as it should have been from the beginning. Of course, that sort of API change can't be made in Asterisk 1.8 or 10, so a note that strongly encourages people to treat the field as such would do well. For trunk, though, I like Richard's idea of improving APIs to reflect what they really should have.

Also, *BARF* at chan_phone. With or without Richard's change, it is making terrible assumptions about the length of the data passed in.


- Mark


-----------------------------------------------------------
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/4dccb1ea/attachment-0001.htm>