[Asterisk-code-review] chan sip: bigger buffers for headers, better failure mode (asterisk[master])

[Corey Farrell]

Corey Farrell has posted comments on this change.

Change subject: chan_sip: bigger buffers for headers, better failure mode
......................................................................


Patch Set 2: Code-Review-1

(9 comments)

https://gerrit.asterisk.org/#/c/2923/2//COMMIT_MSG
Commit Message:

Line 12: resizes them if needed up to SIPHEADERMAX.
"resizes them as needed." - since we're not going to have SIPHEADERMAX.


Line 14: ASTERISK-26069
Unless more changes are needed to fix the original bug this line should be "ASTERISK-26069 #close".


https://gerrit.asterisk.org/#/c/2923/2/channels/chan_sip.c
File channels/chan_sip.c:

Line 14159: #define SIPHEADERMAX 1024
Since we're using alloc'ed strings that can be expanded I'm not sure why we would want this arbitrary limit.  I think just pass 0 to all calls of ast_str_set so they will expand as needed.


Line 14268: 	if (ret < 0 || ret >= SIPHEADERMAX) { /* a return value of size or more means that the output was truncated */
Take a look at the documentation for ast_str_set, the return is different from snprintf.  You only need to check for ret < 0.


PS2, Line 14271: 		/* XXX Make sure the field is terminated with >, to make sure we don't actually create a totally broken packet 
               : 		   See https://issues.asterisk.org/jira/browse/ASTERISK-26069
               : 		*/
               : 		ast_str_append(&from, SIPHEADERMAX+1, ">");
Once you remove the SIPHEADERMAX limitation this will be unneeded.


Line 14281: 		size_t written = strlen(from_buf);
Use ast_str_strlen(from) now that you've switch the type.


PS2, Line 14282: 		ssize_t left = SIPHEADERMAX - written - 4; /* '"" \0' */
               : 		if (left > 0) {
If we do not use SIPHEADERMAX this check and the 'left' variable are unneeded.


Line 14293: 			ast_str_make_space(&from, name_len + written + 4);
This call can fail on allocation error (return non-zero).  Need to check the result.


PS2, Line 14363: 		/* XXX Make sure the field is terminated with >, to make sure we don't actually create a totally broken packet 
               : 		   See https://issues.asterisk.org/jira/browse/ASTERISK-26069
               : 		*/
               : 		ast_str_append(&to, SIPHEADERMAX+1, ">");
This append also not needed.


-- 
To view, visit https://gerrit.asterisk.org/2923
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b68fcbddca6f6cc7d7a92fe1cb0d5430282b2b3
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Vasil Kolev <vasil.kolev at securax.org>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-HasComments: Yes