[Asterisk-code-review] STUN/netsock2: Fix some valgrind uninitialized memory findings. (asterisk[15])

[Jenkins2]

Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/6221 )

Change subject: STUN/netsock2: Fix some valgrind uninitialized memory findings.
......................................................................

STUN/netsock2: Fix some valgrind uninitialized memory findings.

* netsock2.c: Test the addr->len member first as it may be the only member
initialized in the struct.

* stun.c:ast_stun_handle_packet(): The combinded[] local array could get
used uninitialized by ast_stun_request().  The uninitialized string gets
copied to another location and could overflow the destination memory
buffer.

These valgrind findings were found for ASTERISK_27150 but are not
necessarily a fix for the issue.

Change-Id: I55f8687ba4ffc0f69578fd850af006a56cbc9a57
---
M main/netsock2.c
M main/stun.c
2 files changed, 14 insertions(+), 6 deletions(-)

Approvals:
  Joshua Colp: Looks good to me, but someone else must approve
  Corey Farrell: Looks good to me, but someone else must approve
  Sean Bright: Looks good to me, approved
  Jenkins2: Approved for Submit



diff --git a/main/netsock2.c b/main/netsock2.c
index 8fb9c9e..53a43e0 100644
--- a/main/netsock2.c
+++ b/main/netsock2.c
@@ -475,8 +475,12 @@
 
 int ast_sockaddr_is_ipv4(const struct ast_sockaddr *addr)
 {
-	return addr->ss.ss_family == AF_INET &&
-	    addr->len == sizeof(struct sockaddr_in);
+	/*
+	 * Test addr->len first to be tolerant of an ast_sockaddr_setnull()
+	 * addr.  In that case addr->len might be the only value initialized.
+	 */
+	return addr->len == sizeof(struct sockaddr_in)
+		&& addr->ss.ss_family == AF_INET;
 }
 
 int ast_sockaddr_is_ipv4_mapped(const struct ast_sockaddr *addr)
@@ -498,8 +502,12 @@
 
 int ast_sockaddr_is_ipv6(const struct ast_sockaddr *addr)
 {
-	return addr->ss.ss_family == AF_INET6 &&
-	    addr->len == sizeof(struct sockaddr_in6);
+	/*
+	 * Test addr->len first to be tolerant of an ast_sockaddr_setnull()
+	 * addr.  In that case addr->len might be the only value initialized.
+	 */
+	return addr->len == sizeof(struct sockaddr_in6)
+		&& addr->ss.ss_family == AF_INET6;
 }
 
 int ast_sockaddr_is_any(const struct ast_sockaddr *addr)
diff --git a/main/stun.c b/main/stun.c
index 77ced82..c103ab8 100644
--- a/main/stun.c
+++ b/main/stun.c
@@ -343,6 +343,8 @@
 			if (st.username) {
 				append_attr_string(&attr, STUN_USERNAME, st.username, &resplen, &respleft);
 				snprintf(combined, sizeof(combined), "%16s%16s", st.username + 16, st.username);
+			} else {
+				combined[0] = '\0';
 			}
 
 			append_attr_address(&attr, STUN_MAPPED_ADDRESS, src, &resplen, &respleft);
@@ -398,8 +400,6 @@
 	stun_req_id(req);
 	reqlen = 0;
 	reqleft = sizeof(req_buf) - sizeof(struct stun_header);
-	req->msgtype = 0;
-	req->msglen = 0;
 	attr = (struct stun_attr *) req->ies;
 	if (username) {
 		append_attr_string(&attr, STUN_USERNAME, username, &reqlen, &reqleft);

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

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: merged
Gerrit-Change-Id: I55f8687ba4ffc0f69578fd850af006a56cbc9a57
Gerrit-Change-Number: 6221
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170814/744e59cb/attachment.html>