[Asterisk-code-review] fix: memory leaks, resource leaks, out of bounds and bugs (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: fix: memory leaks, resource leaks, out of bounds and bugs
......................................................................


Patch Set 5: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/#/c/3036/5/main/ast_expr2.y
File main/ast_expr2.y:

Line 1664: 	
Since ast_expr2.c is generated from this file, you need to fix this red blob too.  You fixed it in the ast_expr2.c file.


https://gerrit.asterisk.org/#/c/3036/5/main/say.c
File main/say.c:

PS5, Line 7951:                              sndoffset=0;
              :                              for (sndoffset = 0 ; (format[++offset] != '\'') && (sndoffset < sizeof(sndfile) - 1) ; sndoffset++) {
sndoffset is initialized twice here.


https://gerrit.asterisk.org/#/c/3036/5/res/ael/pval.c
File res/ael/pval.c:

PS5, Line 3358: 	RAII_VAR(char *, buf1, NULL, ast_free);
              : 	RAII_VAR(char *, buf2, NULL, ast_free);
              : 	RAII_VAR(char *, new_label, NULL, ast_free);
Asthetically you should be using free instead to match the malloc's.


PS5, Line 5432: 	if (!hr || !dom || !dow || !mon || !s) {
              : 		destroy_pval(hr);
              : 		destroy_pval(dom);
              : 		destroy_pval(dow);
              : 		destroy_pval(mon);
              : 		destroy_pval(s);
              : 	}
Missing return


PS5, Line 5699: 	if (!hr || !dom || !dow || !mon) {
              : 		destroy_pval(hr);
              : 		destroy_pval(dom);
              : 		destroy_pval(dow);
              : 		destroy_pval(mon);
              : 	}
missing return


https://gerrit.asterisk.org/#/c/3036/5/res/res_pjsip_sdp_rtp.c
File res/res_pjsip_sdp_rtp.c:

PS5, Line 422: 		tmp = strchr(ast_str_buffer(fmtp0), ':');
             : 		if (tmp && tmp[1] != '\0') {
             : 			fmtp1 = pj_str(tmp + 1);
             : 		} else {
             : 			fmtp1 = pj_str(ast_str_buffer(fmtp0));
             : 		}
             : 		attr = pjmedia_sdp_attr_create(pool, "fmtp", &fmtp1);
This is not a finding against the patch itself but a comment about this part of the existing code.

ast_format_generate_sdp_fmtp() returns a string that begins: a=fmtp:... if the format has any attributes to declare.

This code is weird.  I think the conversion this routine is a part of is slightly faulty.  For the silk format, ast_format_generate_sdp_fmtp() will return two or three "a=fmtp" attribute lines in the string.  These extra attributes do not get broken up into separate pjmedia_sdp_attr entries.  However, I think that this is benign as the attribute value string containing the extra a=fmtp lines passed to pjproject should be handled transparently.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecbf7d0f360a021147344c4e83ab242fd1e7512c
Gerrit-PatchSet: 5
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Scott Griepentrog <sgriepentrog at digium.com>
Gerrit-HasComments: Yes