<p>George Joseph <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/6413">View Change</a></p><p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(4 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/6413/2/res/res_rtp_asterisk.c">File res/res_rtp_asterisk.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6413/2/res/res_rtp_asterisk.c@5374">Patch Set #2, Line 5374:</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;">Ast1 may see an initial RTP stream from<br> * Ast2 and then a stream from PartyB. Since Ast1 may not have<br> * seen or learned the RTP stream from Ast2 before being told to learn<br> * the stream from PartyB, Ast1 may lock onto the Ast2 stream and<br> * never learn the PartyB stream.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd just replace this with "Ast1 may lock onto the Ast2 stream and never learn the PartyB stream when it starts."</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6413/2/res/res_rtp_asterisk.c@5382">Patch Set #2, Line 5382:</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;"> * To mitigate against attack, the learning state cannot switch<br> * streams while there are competing streams. The competing streams<br> * interfere with each other's qualification. Once we accept a<br> * stream and reach the timeout, an attacker cannot interfere<br> * anymore.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">So as long as there are competing streams, we'll lock onto none of them right? Then what happens? What's the user experience in this case?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6413/2/res/res_rtp_asterisk.c@5400">Patch Set #2, Line 5400:</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;">learning for the new source so it only takes over once<br> * sufficient traffic has been received.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">As long as we receive packets from the original source, we keep resetting the state for the new source so it can't reach the minimum packets and be locked to, right?</p><p style="white-space: pre-wrap; word-wrap: break-word;">So I'd add "...and no more packets have been received from the original source."</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6413/2/res/res_rtp_asterisk.c@5407">Patch Set #2, Line 5407:</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;"> /*<br> * We give preferential treatment to the requested target address<br> * (negotiated SDP address) where we are to send our RTP. However,<br> * the other end has no obligation to send from that address even<br> * though it is practically a requirement when NAT is involved.<br> */<br> ast_rtp_instance_get_requested_target_address(instance, &target_address);<br> if (!ast_sockaddr_cmp(&target_address, &addr)) {<br> /* Accept the negotiated target RTP stream as the source */<br> ast_verb(4, "%p -- Strict RTP switching to RTP target address %s as source\n",<br> rtp, ast_sockaddr_stringify(&addr));<br> ast_sockaddr_copy(&rtp->strict_rtp_address, &addr);<br> rtp_learning_seq_init(&rtp->rtp_source_learn, seqno);<br> break;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">How preferential? Maybe this should be before the old/new source check?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/6413">change 6413</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/6413"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ia2d3aa6e0f22906c25971e74f10027d96525f31c </div>
<div style="display:none"> Gerrit-Change-Number: 6413 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Richard Mudgett <rmudgett@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-Comment-Date: Fri, 15 Sep 2017 14:41:00 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>