<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/5760">View Change</a></p><p>Patch set 4:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(8 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/5760/4/channels/chan_pjsip.c">File channels/chan_pjsip.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/4/channels/chan_pjsip.c@451">Patch Set #4, Line 451:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> struct ast_format_cap *cap1;<br> struct ast_format_cap *cap2;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">cap1 and cap2 are ref leaked.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/4/channels/chan_pjsip.c@515">Patch Set #4, Line 515:</a> <code style="font-family:monospace,monospace"> ast_channel_nativeformats_set(chan, caps);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_channel_stage_snapshot(chan) should be moved to just before this call so we don't hangup chan with the snapshot staging flag set.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/4/channels/chan_pjsip.c@2252">Patch Set #4, Line 2252:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> *cause = req_data.cause;<br> return NULL;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">req_data.topology is leaked here.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5760/4/channels/pjsip/dialplan_functions.c">File channels/pjsip/dialplan_functions.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/4/channels/pjsip/dialplan_functions.c@936">Patch Set #4, Line 936:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> size = strlen(ast_format_get_name(fmt)) + 1;<br> if (*len < size) {<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ooh. Pre-existing bug. We aren't considering the null terminator when appending even though we eventually build a string that removes the trailing comma. We can write a null terminator one over the remaining buffer size.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The bug still isn't fixed by this patch either.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/4/channels/pjsip/dialplan_functions.c@955">Patch Set #4, Line 955:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> int i;<br><br> for (i = 0; i < ast_stream_topology_get_count(session->req_topology); ++i) {<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to set<br>buf[0] = '\0';<br>to ensure that buf is empty if there happen to be no streams in the topology or all streams happen to be declined.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/4/channels/pjsip/dialplan_functions.c@963">Patch Set #4, Line 963:</a> <code style="font-family:monospace,monospace"> if (ast_stream_get_type(stream) != media_type) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to also skip declined streams. ast_stream_get_state(stream) == AST_STREAM_STATE_REMOVED</p><p style="white-space: pre-wrap; word-wrap: break-word;">Declined streams may not even have a format cap.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5760/4/include/asterisk/res_pjsip.h">File include/asterisk/res_pjsip.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/4/include/asterisk/res_pjsip.h@668">Patch Set #4, Line 668:</a> <code style="font-family:monospace,monospace"> struct ast_stream_topology *codecs;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this struct member should be left the way it was. This patch spends a lot of effort converting this topology to and from a caps struct. Also this should be the configured codecs for allowed new audio/video streams. Unless we are going to add a lot of configuration options to configure individually allowed streams such as audio1=alaw,ulaw and audio2=ilbc,g729 this topology change is overkill.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5760/4/res/res_pjsip_session.c">File res/res_pjsip_session.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/4/res/res_pjsip_session.c@1818">Patch Set #4, Line 1818:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (ast_stream_topology_append_stream(session->req_topology, clone_stream) < 0) {<br> continue;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">clone_stream is leaked here.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/5760">change 5760</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/5760"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I0358ea3108e2c06ae43308abddf216a23deccb90 </div>
<div style="display:none"> Gerrit-Change-Number: 5760 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Mark Michelson <mmichelson@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Mark Michelson <mmichelson@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 14 Jun 2017 20:44:53 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>