[asterisk-dev] [Code Review] Fix problems when RTP packet frame size is changed

[Kevin Fleming]

> On 2009-03-04 19:21:10, vadim wrote:
> > /branches/1.4/main/frame.c, line 151
> > <http://reviewboard.digium.com/r/184/diff/1/?file=2952#file2952line151>
> >
> >     I wonder, if this code is secure against s->data buffer overflow

It is, but only because the function that is the source of frame to be added (__ast_smoother_feed) has already checked for that condition before passing the frame to this function, or storing it in ->opt to be later used by ast_smoother_reconfigure. Since this function is not an exposed part of the API, I'm OK with leaving the buffer protection to the caller.


> On 2009-03-04 19:21:10, vadim wrote:
> > /branches/1.4/main/frame.c, line 166
> > <http://reviewboard.digium.com/r/184/diff/1/?file=2952#file2952line166>
> >
> >     assert(size < SMOOTHER_SIZE) ?

These are valid concerns; the entire smoother API does not protect the user against requesting a size that cannot be delivered. This will not result in any buffer overflows, but ast_smoother_read() will never return a frame, so the code would fail to work. However, fixing this would require changing the API in incompatible ways, which wouldn't be appropriate for this branch (or any except trunk). I'll consider improving the API if this code gets merged to trunk.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/184/#review515
-----------------------------------------------------------


On 2009-03-04 17:32:49, Kevin Fleming wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/184/
> -----------------------------------------------------------
> 
> (Updated 2009-03-04 17:32:49)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> During some code analysis, I found that calling ast_rtp_codec_setpref() on an ast_rtp session does not work as expected; it does not adjust the smoother that may on the RTP session, in fact it summarily drops it, even if it has data in it, even if the current format's framing size has not changed. This is not good.
> 
> This patch changes this behavior, so that if the packetization size for the current format changes, any existing smoother is safely updated to use the new size, and if no smoother was present, one is created. A new API call for smoothers, ast_smoother_reconfigure(), was required to implement these changes.
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/include/asterisk/frame.h 180297 
>   /branches/1.4/main/frame.c 180297 
>   /branches/1.4/main/rtp.c 180297 
> 
> Diff: http://reviewboard.digium.com/r/184/diff
> 
> 
> Testing
> -------
> 
> Compile testing only.
> 
> 
> Thanks,
> 
> Kevin
> 
>