[Asterisk-code-review] res_geolocation: eprofile, parsing, tests and more (asterisk[development/16/geolocation])

[George Joseph]

Attention is currently required from: Joshua Colp.
George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18127 )

Change subject: res_geolocation: eprofile, parsing, tests and more
......................................................................


Patch Set 3:

(4 comments)

File include/asterisk/res_geolocation.h:

https://gerrit.asterisk.org/c/asterisk/+/18127/comment/3f6b4d92_e88383cd 
PS3, Line 50: 		AST_STRING_FIELD(method);
> Are you expecting lots of additional fields here?

No but "method" isn't really defined in the spec so I don't know how long it will be.  That's why I made it a string field.  Well, that and I couldn't remember if sorcery would take care of allocating and freeing a simple char *.


File res/res_geolocation/geoloc_eprofile.c:

https://gerrit.asterisk.org/c/asterisk/+/18127/comment/2d0bc03f_b6c71e73 
PS3, Line 50: 
> Add a comment that this is safe because stringfields handles NULL

wilco.


https://gerrit.asterisk.org/c/asterisk/+/18127/comment/ccf24539_e3d323ec 
PS3, Line 101: 	eprofile->location_refinement = profile->location_refinement ? ast_variables_dup(profile->location_refinement) : NULL;
> If any of this duplication fails, won't the effective profile be left in a bit of an undefined/not correct state?
> 
> I think we should not allow things to get in that state.

Yeah true.


https://gerrit.asterisk.org/c/asterisk/+/18127/comment/6e8ed657_2e8c45b8 
PS3, Line 122: 	if (ast_strlen_zero(uri)) {
> Are we confident in this simple URI handling here?

I had thought about applying the same rules we're doing for stir/shaken but we're not ever actually doing anything with that uri. The uri scheme is also not just http or https.  It could be "sip", "sips", "pres" or any other scheme for that matter.   It's just an opaque string to us and god help us if we accidentally misidentified it as malicious and dropped it.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18127
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: development/16/geolocation
Gerrit-Change-Id: I50b66bd041b2a62ab329406f20dbaeef1fa68fc1
Gerrit-Change-Number: 18127
Gerrit-PatchSet: 3
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Wed, 09 Mar 2022 15:49:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220309/f7b5f8ae/attachment-0001.html>