[Asterisk-code-review] res srtp: Enable AES-256 and AES-GCM. (asterisk[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change.

Change subject: res_srtp: Enable AES-256 and AES-GCM.
......................................................................


Patch Set 2: Code-Review-1

(4 comments)

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

PS2, Line 13236: 		if (ast_strlen_zero(orig_crypto)) {
               : 			return NULL;
               : 		}
If 'orig_crypto' is empty do you want to return here or simply continue to the next iteration?

If you return then 'a_crypto' is potentially leaked.


PS2, Line 13239: if (ast_asprintf(&a_crypto, "%sa=crypto:%s\r\n", copy, orig_crypto) == -1) {
               : 			return NULL;
               : 		}
Potential memory leak of 'a_crypto' in this off nominal path.


PS2, Line 13232: 	do {
               : 		char *copy = a_crypto;
               : 
               : 		orig_crypto = ast_sdp_srtp_get_attrib(tmp, dtls_enabled, default_taglen_32);
               : 		if (ast_strlen_zero(orig_crypto)) {
               : 			return NULL;
               : 		}
               : 		if (ast_asprintf(&a_crypto, "%sa=crypto:%s\r\n", copy, orig_crypto) == -1) {
               : 			return NULL;
               : 		}
               : 	} while ((tmp = AST_LIST_NEXT(tmp, sdp_srtp_list)));
I think there is potential for a memory leak in this code. asprintf always returns newly allocated memory, which 'a_crypto' points to. No memory is leaked if there is only a single pass on the loop. However, on a second run through 'copy' now also points to the allocated memory created on the first pass. asprintf is again called, which allocates new memory pointed to by 'a_crypto'. At this point there are two allocated strings, 'a_crypto' and 'copy'. However the memory pointed to by 'copy' never gets freed. The more loops through the more memory leaked.


https://gerrit.asterisk.org/#/c/3183/2/main/sdp_srtp.c
File main/sdp_srtp.c:

PS2, Line 612: sizeof(attr) / sizeof(attr[0]);
Should be able to use the ARRAY_LEN macro here found in utils.h


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I11326d80edd656524a51a19450e586c583aa0a0b
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-HasComments: Yes