<p> Attention is currently required from: Kevin Harwell. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/16497">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/res_stir_shaken.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16497/comment/4bc4e0a4_d76f6ff4">Patch Set #3, Line 94:</a> <code style="font-family:monospace,monospace">int ast_stir_shaken_verify(const char *header, const char *payload, const char *signature, const char *algorithm,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">The altered function definition breaks ABI</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yeah, I'm not sure what to do about this honestly. It needs to be changed, and could even change again to have a response "reason" populated as well. Or return the payload and if payload is null on return, have a code / reason combo or struct that gets populated that you can check. Basically, we need a way to distinguish between "did this fail for valid reasons" or "did this fail because something was malformed". The way it is now, it's impossible to distinguish between the two because that logic that needs to be tested for response codes is nested inside of this function.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_pjsip_session.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16497/comment/e8256412_36bccdcc">Patch Set #3, Line 4070:</a> <code style="font-family:monospace,monospace">ast_sip_rdata_get_header_value</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I was curious what this function returned. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Commented on this in other thread. Feel free to correct my thinking if it's wrong.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_pjsip_stir_shaken.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16497/comment/a531733f_d116cae9">Patch Set #3, Line 126:</a> <code style="font-family:monospace,monospace">      char timezone[80];</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Initialize this to NULL just in case, i.e. timezone[80] = {0}. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16497/comment/b9141aed_ed4bea39">Patch Set #3, Line 129:</a> <code style="font-family:monospace,monospace">    date_hdr_val = ast_sip_rdata_get_header_value(rdata, date_hdr_str);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">The return value of 'ast_sip_rdata_get_header_value' needs to be fixed somehow. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">At the end of that function, the string is duplicated with null termination via pj_strdup_with_null, so it should be null-terminated. That function does an alloc and memcpy, so I think this should be ok unless I'm missing something.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16497/comment/e5946700_9683b83d">Patch Set #3, Line 140:</a> <code style="font-family:monospace,monospace"> sscanf(remainder, "%79s", timezone);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is remainder always guaranteed to be the expected timezone? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This was actually snatched from pbx.c. It's possible timezone could be an empty string. In that case, I think checking to see if sscanf populated timezone is all you can really do. Think that's the best approach?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16497/comment/c5da989e_cbb0ec09">Patch Set #3, Line 199:</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;">     RAII_VAR(struct ast_json *, json, NULL, ast_json_unref);<br>      RAII_VAR(char *, combined_str, NULL, ast_free);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Maybe I am missing something, but these to variables appear to be unused. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Leftovers from another implementation that didn't get cleaned up!</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/16497">change 16497</a>. To unsubscribe, or for help writing mail filters, 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/c/asterisk/+/16497"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: I4ac1ecf652cd0e336006b0ca638dc826b5b1ebf7 </div>
<div style="display:none"> Gerrit-Change-Number: 16497 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Attention: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 27 Sep 2021 13:43:34 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>