<p>Patch set 4:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14220">View Change</a></p><p>19 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c">File res/res_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/+/14220/4/res/res_stir_shaken.c@97">Patch Set #4, Line 97:</a> <code style="font-family:monospace,monospace">struct curl_cb_data *data)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">looks like data is unmodified in this function. I recommend making it const</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@106">Patch Set #4, Line 106:</a> <code style="font-family:monospace,monospace">      value = ast_strdup(curl_cb_data_get_cache_control(data));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">value is leaked. Actually no reason to dupe the string here as it's not modified.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@118">Patch Set #4, Line 118:</a> <code style="font-family:monospace,monospace">(sscanf(equal + 1, "%30u", &max_age) == 1)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">replace this with a call to "ast_str_to_uint" instead.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@123">Patch Set #4, Line 123:</a> <code style="font-family:monospace,monospace">                value = ast_strdup(curl_cb_data_get_expires(data));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">value is leaked here too. Actually it appears value is not modified after this so probably don't need to dupe the string at all.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@160">Patch Set #4, Line 160:</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 (sscanf(expiration, "%lu", &expires.tv_sec) != 1) {<br>         return 1;<br>     }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">can probably use "ast_str_to_ulong" or ast_str_to_umax" here instead? If not maybe as a separate patch add a new conversion?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@278">Patch Set #4, Line 278:</a> <code style="font-family:monospace,monospace">   ast_base64decode(decoded_signature, signature, decoded_signature_length);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The return value here would should be the actual written length, so can probably set decoded_signature_length to it and then pass it as the length below.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@280">Patch Set #4, Line 280:</a> <code style="font-family:monospace,monospace">strlen((const char *)decoded_signature)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can you just pass "decoded_signature_length" here?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@363">Patch Set #4, Line 363:</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;">    snprintf(stir_shaken_dir, sizeof(stir_shaken_dir), "%s/keys/%s", ast_config_AST_DATA_DIR, STIR_SHAKEN_DIR_NAME);<br>    filename = basename(public_key_url);<br>  snprintf(default_path, sizeof(default_path), "%s/%s", stir_shaken_dir, filename);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Move this after checking the db. If in db no reason to retrieve and copy the default path.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@377">Patch Set #4, Line 377:</a> <code style="font-family:monospace,monospace">  snprintf(file_path, sizeof(file_path), "%s", get_path_to_public_key(public_key_url));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This leaks the returned value from get_path_to_public_key. Does this need to even be copied? If so why not just strdup?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@395">Patch Set #4, Line 395:</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;">snprintf(file_path, sizeof(file_path), "%s", default_path);<br><br>               /* We should have a successful download at this point, so<br>              * add an entry to the database.<br>               */<br>           add_public_key_to_astdb(public_key_url, file_path);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">No reason to duplicate the file_path here as you can just pass the default directly to the "add_public_key_to_astdb" function. Or is file_path suppose to have appended something to the default_path?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@479">Patch Set #4, Line 479:</a> <code style="font-family:monospace,monospace">    json_header = ast_json_pack(header);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If you wanted you could just set ret_payload->header = ast_json_(...), and remove the extra variable.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also does passing in the base string only work here? I thought you always had to pass a format specifier as the first parameter?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@487">Patch Set #4, Line 487:</a> <code style="font-family:monospace,monospace">    json_payload = ast_json_pack(payload);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If you wanted you could just set ret_payload->payload = ast_json_(...), and remove the extra variable.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also does passing in the base string only work here? I thought you always had to pass a format specifier as the first parameter?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@495">Patch Set #4, Line 495:</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;">    ret_payload->signature = (unsigned char *)ast_strdupa(signature);<br>  ret_payload->algorithm = ast_strdupa(algorithm);<br>   ret_payload->public_key_url = ast_strdupa(public_key_url)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">These should all use ast_strdup, and not ast_strdupa.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@893">Patch Set #4, Line 893:</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;">#undef AST_BUILDOPT_SUM<br>#define AST_BUILDOPT_SUM ""<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Just noticed this too. But I _think_ this is not needed, and can be safely deleted.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken.c@896">Patch Set #4, Line 896:</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;">AST_MODULE_INFO(ASTERISK_GPL_KEY, AST_MODFLAG_GLOBAL_SYMBOLS | AST_MODFLAG_LOAD_ORDER,<br>                          "STIR/SHAKEN Module for Asterisk",<br>  .support_level = AST_MODULE_SUPPORT_CORE,<br>     .load = load_module,<br>  .unload = unload_module,<br>      .reload = reload_module,<br>      .load_pri = AST_MODPRI_CHANNEL_DEPEND - 1,<br>);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">this needs a: .requires = "res_curl"</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken/curl.c">File res/res_stir_shaken/curl.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/+/14220/4/res/res_stir_shaken/curl.c@112">Patch Set #4, Line 112:</a> <code style="font-family:monospace,monospace">                cb_data->cache_control = ast_strdupa(value);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be ast_strdup.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken/curl.c@113">Patch Set #4, Line 113:</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;">        } else if (!strcasecmp(header, "Expires")) {<br>                cb_data->cache_control = ast_strdupa(value);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be ->expires = ast_strdup(value)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken/curl.c@180">Patch Set #4, Line 180:</a> <code style="font-family:monospace,monospace">            return -1;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I believe you need to call cur_easy_cleanup here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14220/4/res/res_stir_shaken/stir_shaken.h">File res/res_stir_shaken/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/+/14220/4/res/res_stir_shaken/stir_shaken.h@53">Patch Set #4, Line 53:</a> <code style="font-family:monospace,monospace">EVP_PKEY *read_key(const char *path, int priv);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">rename this to stir_shaken_read_key to avoid potential name collision and easier searching.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/14220">change 14220</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/+/14220"/><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-Change-Id: I3ba4c63880493bf8c7d17a9cfca1af0e934d1a1c </div>
<div style="display:none"> Gerrit-Change-Number: 14220 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </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: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-CC: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 22 Apr 2020 22:59:51 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>