[asterisk-dev] [Code Review] 3674: media_formats: remove ast_format_copy, directly use ao2_bump or ao2_replace

[Corey Farrell]

> On June 26, 2014, 6:54 a.m., Joshua Colp wrote:
> > /team/group/media_formats-reviewed-trunk/main/channel.c, line 5309
> > <https://reviewboard.asterisk.org/r/3674/diff/1/?file=60929#file60929line5309>
> >
> >     I don't understand the BUGBUG

It seemed weird to me that the parameters are being used like normal variables, but now I see that the input variable was used above here.  I'll remove this comment.


> On June 26, 2014, 6:54 a.m., Joshua Colp wrote:
> > /team/group/media_formats-reviewed-trunk/main/translate.c, lines 1361-1366
> > <https://reviewboard.asterisk.org/r/3674/diff/1/?file=60936#file60936line1361>
> >
> >     Agreed. Interesting re: valgrind...
> >     
> >     What if the variables passed in weren't initialized at the start to NULL?

I believe some cases we have:
struct ast_format *dst_fmt;
struct ast_format *src_fmt;
ast_translator_best_choice(dst_cap, src_cap, &dst_fmt, &src_fmt);

Where ao2_replace on *dst_fmt_out would cause undefined memory access.  I suspect that in other cases dst_fmt or src_fmt is initialized and holding a reference (thus the leak).  I can't say for sure but I've traced some leaking that I think has to do with this procedure, and this ao2_bump seems to me like the most likely suspect.


- Corey


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


On June 25, 2014, 5:38 p.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3674/
> -----------------------------------------------------------
> 
> (Updated June 25, 2014, 5:38 p.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp and Matt Jordan.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Remove ast_format_copy, make use of ao2_bump directly.  Use ao2_replace where appropriate.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed-trunk/tests/test_core_format.c 417232 
>   /team/group/media_formats-reviewed-trunk/res/res_speech.c 417232 
>   /team/group/media_formats-reviewed-trunk/res/res_rtp_asterisk.c 417232 
>   /team/group/media_formats-reviewed-trunk/res/res_musiconhold.c 417232 
>   /team/group/media_formats-reviewed-trunk/res/res_fax.c 417232 
>   /team/group/media_formats-reviewed-trunk/res/res_agi.c 417232 
>   /team/group/media_formats-reviewed-trunk/res/res_adsi.c 417232 
>   /team/group/media_formats-reviewed-trunk/main/translate.c 417232 
>   /team/group/media_formats-reviewed-trunk/main/smoother.c 417232 
>   /team/group/media_formats-reviewed-trunk/main/slinfactory.c 417232 
>   /team/group/media_formats-reviewed-trunk/main/rtp_engine.c 417232 
>   /team/group/media_formats-reviewed-trunk/main/indications.c 417232 
>   /team/group/media_formats-reviewed-trunk/main/frame.c 417232 
>   /team/group/media_formats-reviewed-trunk/main/file.c 417232 
>   /team/group/media_formats-reviewed-trunk/main/channel.c 417232 
>   /team/group/media_formats-reviewed-trunk/main/bridge_channel.c 417232 
>   /team/group/media_formats-reviewed-trunk/main/audiohook.c 417232 
>   /team/group/media_formats-reviewed-trunk/main/app.c 417232 
>   /team/group/media_formats-reviewed-trunk/main/abstract_jb.c 417232 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/slin.h 417232 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format.h 417232 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_ulaw.h 417232 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_speex.h 417232 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_lpc10.h 417232 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_ilbc.h 417232 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_gsm.h 417232 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_g726.h 417232 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_g722.h 417232 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_alaw.h 417232 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_adpcm.h 417232 
>   /team/group/media_formats-reviewed-trunk/codecs/codec_dahdi.c 417232 
>   /team/group/media_formats-reviewed-trunk/channels/chan_phone.c 417232 
>   /team/group/media_formats-reviewed-trunk/channels/chan_oss.c 417232 
>   /team/group/media_formats-reviewed-trunk/channels/chan_misdn.c 417232 
>   /team/group/media_formats-reviewed-trunk/channels/chan_dahdi.c 417232 
>   /team/group/media_formats-reviewed-trunk/channels/chan_alsa.c 417232 
>   /team/group/media_formats-reviewed-trunk/bridges/bridge_softmix.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_waitforsilence.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_test.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_talkdetect.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_speech_utils.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_sms.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_record.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_nbscat.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_mp3.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_milliwatt.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_ices.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_festival.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_fax.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_dictate.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_dahdibarge.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_chanspy.c 417232 
>   /team/group/media_formats-reviewed-trunk/apps/app_amd.c 417232 
>   /team/group/media_formats-reviewed-trunk/addons/chan_ooh323.c 417232 
>   /team/group/media_formats-reviewed-trunk/addons/chan_mobile.c 417232 
> 
> Diff: https://reviewboard.asterisk.org/r/3674/diff/
> 
> 
> Testing
> -------
> 
> Compile only.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140626/894fd45e/attachment-0001.html>