<p> Attention is currently required from: Kevin Harwell. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/18068">View Change</a></p><p>24 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/res_geolocation.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/+/18068/comment/24b072ad_041a8c56">Patch Set #5, Line 24:</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;">enum geoloc_pidf_section {<br> PIDF_SECTION_NONE = 0,<br> PIDF_SECTION_TUPLE,<br> PIDF_SECTION_DEVICE,<br> PIDF_SECTION_PERSON<br>};<br><br>enum geoloc_format {<br> GEOLOC_FORMAT_NONE = 0,<br> GEOLOC_FORMAT_CIVIC_ADDRESS,<br> GEOLOC_FORMAT_GML,<br> GEOLOC_FORMAT_URI,<br>};<br><br>enum geoloc_location_disposition {<br> GEOLOC_LOC_DISP_DISCARD = 0,<br> GEOLOC_LOC_DISP_APPEND,<br> GEOLOC_LOC_DISP_PREPEND,<br> GEOLOC_LOC_DISP_REPLACE,<br>};<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should these enums start with "ast_" since they're public?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/783cb950_b51dec5c">Patch Set #5, Line 24:</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;">enum geoloc_pidf_section {<br> PIDF_SECTION_NONE = 0,<br> PIDF_SECTION_TUPLE,<br> PIDF_SECTION_DEVICE,<br> PIDF_SECTION_PERSON<br>};<br><br>enum geoloc_format {<br> GEOLOC_FORMAT_NONE = 0,<br> GEOLOC_FORMAT_CIVIC_ADDRESS,<br> GEOLOC_FORMAT_GML,<br> GEOLOC_FORMAT_URI,<br>};<br><br>enum geoloc_location_disposition {<br> GEOLOC_LOC_DISP_DISCARD = 0,<br> GEOLOC_LOC_DISP_APPEND,<br> GEOLOC_LOC_DISP_PREPEND,<br> GEOLOC_LOC_DISP_REPLACE,<br>};<br><br>struct ast_geoloc_location {<br> SORCERY_OBJECT(details);<br> enum geoloc_format format;<br> struct ast_variable *location_vars;<br>};<br><br>struct ast_geoloc_profile {<br> SORCERY_OBJECT(details);<br> AST_DECLARE_STRING_FIELDS(<br> AST_STRING_FIELD(location_reference);<br> );<br> enum geoloc_pidf_section pidf_section;<br> enum geoloc_location_disposition location_disposition;<br> int geolocation_routing;<br> int send_location;<br> struct ast_variable *location_refinement;<br> struct ast_variable *location_variables;<br> struct ast_variable *usage_rules_vars;<br>};<br><br>struct ast_geoloc_effective_profile {<br> AST_DECLARE_STRING_FIELDS(<br> AST_STRING_FIELD(id);<br> AST_STRING_FIELD(location_reference);<br> );<br> enum geoloc_pidf_section pidf_section;<br> enum geoloc_location_disposition location_disposition;<br> int geolocation_routing;<br> int send_location;<br> enum geoloc_format format;<br> struct ast_variable *location_vars;<br> struct ast_variable *location_refinement;<br> struct ast_variable *location_variables;<br> struct ast_variable *effective_location;<br> struct ast_variable *usage_rules_vars;<br>};<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">These enums and structs should be documented too, but if waiting to do that later that's fine.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/ada8f2c5_b092c35f">Patch Set #5, Line 110:</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;">enum ast_geoloc_validate_result {<br> AST_GEOLOC_VALIDATE_SUCCESS = 0,<br> AST_GEOLOC_VALIDATE_MISSING_TYPE,<br> AST_GEOLOC_VALIDATE_INVALID_TYPE,<br> AST_GEOLOC_VALIDATE_INVALID_VARNAME,<br> AST_GEOLOC_VALIDATE_NOT_ENOUGH_VARNAMES,<br> AST_GEOLOC_VALIDATE_TOO_MANY_VARNAMES,<br> AST_GEOLOC_VALIDATE_INVALID_VALUE,<br>};<br><br>const char *ast_geoloc_validate_result_to_str(enum ast_geoloc_validate_result result);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">more missing docs.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/93e3b9ba_b92ea095">Patch Set #5, Line 130:</a> <code style="font-family:monospace,monospace">struct ast_variable *varlist</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">make "varlist" const since it's not being modified in the function.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/6c34ca81_bb8b79f2">Patch Set #5, Line 144:</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_datastore *ast_geoloc_datastore_create(const char *profile_name);<br><br>struct ast_geoloc_effective_profile *ast_geoloc_effective_profile_create(struct ast_geoloc_profile *profile);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Docs needed for these functions too.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation.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/+/18068/comment/e497a6e3_08047722">Patch Set #5, Line 46:</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;"> res += geoloc_civicaddr_reload();<br> res += geoloc_gml_reload();<br> res += geoloc_config_reload();<br> res += geoloc_dialplan_reload();<br> res += geoloc_channel_reload();<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If any of these fail the module declines. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation/geoloc_channel.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/+/18068/comment/e7e9dd96_d17d59b2">Patch Set #5, Line 45:</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 (!profile) {<br> ast_log(LOG_ERROR, "A profile with the name '%s' was not found", profile_name);<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should this path free "ds" and return NULL here?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/1dd9aedc_94b457eb">Patch Set #5, Line 49:</a> <code style="font-family:monospace,monospace"> eprofile = ast_geoloc_effective_profile_create(profile);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">As the code is now "profile" can be passed as NULL, which will cause things to crash inside ast_geol […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/53f4077b_f541ae99">Patch Set #5, Line 50:</a> <code style="font-family:monospace,monospace"> ds->data = eprofile;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What if "eprofile" is NULL? Should processing discontinue here or be allowed to continue with a NULL […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation/geoloc_config.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/+/18068/comment/4c81eb3d_29442004">Patch Set #5, Line 59:</a> <code style="font-family:monospace,monospace">ast_sorcery_generic_alloc</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">ast_sorcery_generic_alloc can return NULL so location needs a NULL check.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/fb2c9ded_70a5dfab">Patch Set #5, Line 82:</a> <code style="font-family:monospace,monospace"> struct ast_geoloc_profile *profile = ast_sorcery_generic_alloc(sizeof(*profile), geoloc_profile_destructor);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Same here, it's possible for ast_sorcery_generic_alloc to return NULL so "profile" needs to be check […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/0c8a3e0b_1f61a365">Patch Set #5, Line 113:</a> <code style="font-family:monospace,monospace"> const char *profile_id = ast_sorcery_object_get_id(profile);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">So far there's at least one place profile can be passed in as NULL, so it needs a NULL check before […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/93dcb112_5bb029f6">Patch Set #5, Line 116:</a> <code style="font-family:monospace,monospace"> eprofile = geoloc_effective_profile_alloc(profile_id);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Do a NULL check on "eprofile" after allocation attempt.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/91e01d6f_e1f845e4">Patch Set #5, Line 163:</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_log(LOG_ERROR, "Location '%s' must have a format\n", location_id);<br> return -1;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">These lines can move back a tab.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/091e8302_b35bd105">Patch Set #5, Line 165:</a> <code style="font-family:monospace,monospace"> break;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This line can be removed.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/2d9e526e_ed98d38c">Patch Set #5, Line 233:</a> <code style="font-family:monospace,monospace"> return -1;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">"location" is ref leaked here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/fda80bb0_e24d2caf">Patch Set #5, Line 243:</a> <code style="font-family:monospace,monospace"> return 0;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">"location" is ref leaked here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/89912463_c2acf779">Patch Set #5, Line 586:</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 (geoloc_sorcery) {<br> ast_sorcery_unref(geoloc_sorcery);<br> }<br> geoloc_sorcery = NULL;<br></pre></blockquote></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think is an actual problem (at least highly unlikely), but will mention just in case.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Without locking on geoloc_sorcery, if multiple threads are in play it may be possible for something to call "geoloc_get_sorcery" between the unref and NULL here. If so then "geoloc_get_sorcery" potentially try to ref an invalid pointer.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">This is how pjsip_configuration does it.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation/geoloc_doc.xml:</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/+/18068/comment/6e925c71_b01ae17a">Patch Set #5, Line 10:</a> <code style="font-family:monospace,monospace">cffdffff</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Not sure what this is?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/98e61969_ff6f2c9f">Patch Set #5, Line 51:</a> <code style="font-family:monospace,monospace"> </configObject> </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">extra space at eol.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/e9095d77_3db850e9">Patch Set #5, Line 55:</a> <code style="font-family:monospace,monospace">cffdffff</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/1973ba28_72b871dc">Patch Set #5, Line 89:</a> <code style="font-family:monospace,monospace">xxxx</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Just acting as a placeholder for now?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation/geoloc_gml.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/+/18068/comment/d2ffba69_146004f2">Patch Set #5, Line 24:</a> <code style="font-family:monospace,monospace">#if 1 //not used yet.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If not used yet should this be "#if 0", or if now referenced the "#if" can be removed?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation/geoloc_private.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/+/18068/comment/08c2ef0d_c989f3f4">Patch Set #5, Line 80:</a> <code style="font-family:monospace,monospace"> new_var = ast_variable_new(item_name, item_value, ""); \</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This needs a NULL check.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/18068">change 18068</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/+/18068"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: development/16/geolocation </div>
<div style="display:none"> Gerrit-Change-Id: Ieb6e3640f31a676da42d8c144ebbb31ad795d849 </div>
<div style="display:none"> Gerrit-Change-Number: 18068 </div>
<div style="display:none"> Gerrit-PatchSet: 8 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@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: Tue, 22 Feb 2022 19:49:56 +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>