----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3703/#review12506 -----------------------------------------------------------
/team/group/media_formats-reviewed-trunk/channels/chan_sip.c <https://reviewboard.asterisk.org/r/3703/#comment22772> I think we're going to have a ref leak on format_parsed here. Since ast_format_parse_sdp_fmtp will also return a new format with the reference count increased to 1, the ao2_replace will actually bump this one further than we want. If I'm counting this correctly: 11305: if ((format = ast_rtp_codecs_get_payload_format(newaudiortp, codec))) { /* +1 to format */ 11309: format_parsed = ast_format_parse_sdp_fmtp(format, fmtp_string); /* +1 to format_parsed */ 11311: ast_rtp_codecs_payload_replace_format(newaudiortp, codec, format_parsed); /* Store format_parsed on codecs, +1 to format_parsed */ 11312: ao2_replace(format, format_parsed); /* -1 to format (matches original ref); +1 to format_parsed; format is now format_parsed */ 11346: ao2_ref(format, -1); /* -1 to format_parsed */ Which leaves format_parsed (now format) with a ref count of 1, when it should be 0 (or neutral) after this routine. I think the ao2_replace of format with format_parsed should actually be: ao2_ref(format, -1); format = format_parsed Which would allow the latter ao2_ref on 11346 to dispose of the reference returned by ast_format_parse_sdp_fmtp /team/group/media_formats-reviewed-trunk/channels/chan_sip.c <https://reviewboard.asterisk.org/r/3703/#comment22773> Same potential leak here - Matt Jordan On July 3, 2014, 7:15 a.m., Joshua Colp wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3703/ > ----------------------------------------------------------- > > (Updated July 3, 2014, 7:15 a.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-23957 > https://issues.asterisk.org/jira/browse/ASTERISK-23957 > > > Repository: Asterisk > > > Description > ------- > > This change does a few things: > > 1. Fixes an issue where direct format interfaces were treated as AO2 objects > when they were not. > 2. Added an ast_format_clone API call which clones and deep copies a format, > returning one which can be safely modified. > 3. Changed the format interface API so anything which manipulates the format > returns a new format. > 4. Added get/set functions for format attribute data. > 5. Added an API call to replace the format in an RTP engine codecs structure. > 6. Updated the res_format_attr_* modules to work with the new media formats > work. > 7. Added support for loading an interface module after a format has been > created. > > > Diffs > ----- > > /team/group/media_formats-reviewed-trunk/res/res_pjsip_sdp_rtp.c 417804 > /team/group/media_formats-reviewed-trunk/res/res_format_attr_silk.c 417804 > /team/group/media_formats-reviewed-trunk/res/res_format_attr_opus.c 417804 > /team/group/media_formats-reviewed-trunk/res/res_format_attr_h264.c 417804 > /team/group/media_formats-reviewed-trunk/res/res_format_attr_h263.c 417804 > /team/group/media_formats-reviewed-trunk/res/res_format_attr_celt.c 417804 > /team/group/media_formats-reviewed-trunk/main/rtp_engine.c 417804 > /team/group/media_formats-reviewed-trunk/main/format.c 417804 > /team/group/media_formats-reviewed-trunk/include/asterisk/rtp_engine.h > 417804 > /team/group/media_formats-reviewed-trunk/include/asterisk/format.h 417804 > /team/group/media_formats-reviewed-trunk/channels/chan_sip.c 417804 > > Diff: https://reviewboard.asterisk.org/r/3703/diff/ > > > Testing > ------- > > Placed calls in and out, confirmed that the attribute stuff doesn't crash > things and that received SDP is parsed and interpreted. What doesn't > currently work is passing this information through so outgoing calls have the > correct attributes. > > > Thanks, > > Joshua Colp > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
