[Asterisk-code-review] ASTERISK-24601 Include LOCAL/REMOTE tags in BLF NOTIFY XML Patch Orig... (asterisk[16])

[Joe]

Joe has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/15763 )

Change subject: ASTERISK-24601 Include LOCAL/REMOTE tags in BLF NOTIFY XML Patch Originally submitted by Joshua Elson Modified by Joseph Nadiv and Sean Bright
......................................................................


Patch Set 1:

(3 comments)

> Patch Set 1: Code-Review-1
> 
> (4 comments)
> 
> This is substantial enough that it also warrants a CHANGES entry[1].
> 
> [1] https://wiki.asterisk.org/wiki/display/AST/CHANGES+and+UPGRADE.txt

https://gerrit.asterisk.org/c/asterisk/+/15763/1/res/res_pjsip_dialog_info_body_generator.c 
File res/res_pjsip_dialog_info_body_generator.c:

https://gerrit.asterisk.org/c/asterisk/+/15763/1/res/res_pjsip_dialog_info_body_generator.c@156 
PS1, Line 156: 		from_domain = endpoint ? (!ast_strlen_zero(endpoint->fromdomain) ? endpoint->fromdomain : invalid) : NULL;
> This should have a comment stating that from_domain remains valid as the subscription holds a refere […]
The concern here was that 'endpoint' info may not contain 'fromdomain' info, so in order to prevent and issues we're verifying that information.  If it's NULL then return 'invalid' or NULL.
from_domain will not always return something, it will return NULL if endpoint is invalid.
Are you saying that this check is unnecessary, since endpoint cannot be NULL at this point in the logic, so we can just do this:
from_domain = !ast_strlen_zero(endpoint->fromdomain) ? endpoint->fromdomain : invalid;


https://gerrit.asterisk.org/c/asterisk/+/15763/1/res/res_pjsip_dialog_info_body_generator.c@192 
PS1, Line 192: 			ast_debug(3, "From Domain remote URI domain '%s'\n", sanitized_remote);
> If this debug message is to remain it should be extended with additional information about what this […]
This is really just for debug in development, and does not need to remain in the codebase.


https://gerrit.asterisk.org/c/asterisk/+/15763/1/res/res_pjsip_dialog_info_body_generator.c@207 
PS1, Line 207: 			need = strlen(connected_num) + (connected_num_restricted ? strlen(invalid) :
> remote_target has a fixed maximum size, I don't see why this needs to be calculated and given to snp […]
I didn't write this line, I presume it's to prevent injection and buffer overflow.  remote_target max size is the limitation of PJSIP, but once we know the actual length of our data it would probably be better to set it first. Is there a reason to change this?



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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I20c5cf5b45f34d7179df6573c5abf863eb72964b
Gerrit-Change-Number: 15763
Gerrit-PatchSet: 1
Gerrit-Owner: Joe <ynadiv at corpit.xyz>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-CC: Friendly Automation
Gerrit-Comment-Date: Wed, 14 Apr 2021 17:33:15 +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/20210414/d36be8f2/attachment.html>