Attachment 3164 Details for Bug 2883 – resolve memory leaks

resolve memory leaks

openssh_coverity.log (text/plain), 7.28 KB, created by Jakub Jelen on 2018-07-19 00:46:44 AEST

(hide)

Description:

Filename:

MIME Type:

Creator: Jakub Jelen

Created: 2018-07-19 00:46:44 AEST

Size: 7.28 KB

patch

obsolete

>From 984749e0d2a2292d51eeb6a18e0fca6ff143e59d Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Wed, 18 Jul 2018 14:19:56 +0200
>Subject: [PATCH 1/4] coverity: RESOURCE_LEAK (CWE-772)
>
>addrmatch.c:217: leaked_storage: Variable "ai" going out of scope leaks the storage it points to.
>  215|
>  216|   	if (ai == NULL || ai->ai_addr == NULL)
>  217|-> 		return -1;
>  218|
>  219|   	if (n != NULL &&
>---
> addrmatch.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/addrmatch.c b/addrmatch.c
>index 8658e105..74433fe0 100644
>--- a/addrmatch.c
>+++ b/addrmatch.c
>@@ -213,9 +213,14 @@ addr_pton(const char *p, struct xaddr *n)
> 	if (p == NULL || getaddrinfo(p, NULL, &hints, &ai) != 0)
> 		return -1;
> 
>-	if (ai == NULL || ai->ai_addr == NULL)
>+	if (ai == NULL)
> 		return -1;
> 
>+	if (ai == NULL || ai->ai_addr == NULL) {
>+		freeaddrinfo(ai);
>+		return -1;
>+	}
>+
> 	if (n != NULL &&
> 	    addr_sa_to_xaddr(ai->ai_addr, ai->ai_addrlen, n) == -1) {
> 		freeaddrinfo(ai);
>-- 
>2.17.1
>
>
>From 188974954591c7e908b12090d4209c1a72b25346 Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Wed, 18 Jul 2018 14:52:56 +0200
>Subject: [PATCH 2/4] coverity: RESOURCE_LEAK (CWE-772)
>
>(this ran for older version, so the code is a bit different now)
>
>openssh-7.7p1/compat.c:212: overwrite_var: Overwriting "p" in "p = match_filter_list(p, "diffie-hellman-group-exchange-sha256,diffie-hellman-group-exchange-sha1")" leaks the storage that "p" points to.
>  210|   			fatal("match_filter_list failed");
>  211|   	if ((datafellows & SSH_OLD_DHGEX) != 0) {
>  212|-> 		if ((p = match_filter_list(p,
>  213|   		    "diffie-hellman-group-exchange-sha256,"
>  214|   		    "diffie-hellman-group-exchange-sha1")) == NULL)
>---
> compat.c | 31 ++++++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
>diff --git a/compat.c b/compat.c
>index d0afe908..2e6942c6 100644
>--- a/compat.c
>+++ b/compat.c
>@@ -186,11 +186,16 @@ proto_spec(const char *spec)
> char *
> compat_cipher_proposal(char *cipher_prop)
> {
>+	char *np = NULL;
>+
> 	if (!(datafellows & SSH_BUG_BIGENDIANAES))
> 		return cipher_prop;
> 	debug2("%s: original cipher proposal: %s", __func__, cipher_prop);
>-	if ((cipher_prop = match_filter_blacklist(cipher_prop, "aes*")) == NULL)
>+	np = match_filter_blacklist(cipher_prop, "aes*");
>+	if (np == NULL)
> 		fatal("match_filter_blacklist failed");
>+	free(cipher_prop);
>+	cipher_prop = np;
> 	debug2("%s: compat cipher proposal: %s", __func__, cipher_prop);
> 	if (*cipher_prop == '\0')
> 		fatal("No supported ciphers found");
>@@ -200,11 +205,16 @@ compat_cipher_proposal(char *cipher_prop)
> char *
> compat_pkalg_proposal(char *pkalg_prop)
> {
>+	char *np = NULL;
>+
> 	if (!(datafellows & SSH_BUG_RSASIGMD5))
> 		return pkalg_prop;
> 	debug2("%s: original public key proposal: %s", __func__, pkalg_prop);
>-	if ((pkalg_prop = match_filter_blacklist(pkalg_prop, "ssh-rsa")) == NULL)
>+	np = match_filter_blacklist(pkalg_prop, "ssh-rsa");
>+	if (np == NULL)
> 		fatal("match_filter_blacklist failed");
>+	free(pkalg_prop);
>+	pkalg_prop = np;
> 	debug2("%s: compat public key proposal: %s", __func__, pkalg_prop);
> 	if (*pkalg_prop == '\0')
> 		fatal("No supported PK algorithms found");
>@@ -217,15 +227,22 @@ compat_kex_proposal(char *p)
> 	if ((datafellows & (SSH_BUG_CURVE25519PAD|SSH_OLD_DHGEX)) == 0)
> 		return p;
> 	debug2("%s: original KEX proposal: %s", __func__, p);
>-	if ((datafellows & SSH_BUG_CURVE25519PAD) != 0)
>-		if ((p = match_filter_blacklist(p,
>-		    "curve25519-sha256@libssh.org")) == NULL)
>+	if ((datafellows & SSH_BUG_CURVE25519PAD) != 0) {
>+		char *np = match_filter_blacklist(p,
>+		    "curve25519-sha256@libssh.org");
>+		if (np == NULL)
> 			fatal("match_filter_blacklist failed");
>+		free(p);
>+		p = np;
>+	}
> 	if ((datafellows & SSH_OLD_DHGEX) != 0) {
>-		if ((p = match_filter_blacklist(p,
>+		char *np = match_filter_blacklist(p,
> 		    "diffie-hellman-group-exchange-sha256,"
>-		    "diffie-hellman-group-exchange-sha1")) == NULL)
>+		    "diffie-hellman-group-exchange-sha1");
>+		if (np == NULL)
> 			fatal("match_filter_blacklist failed");
>+		free(p);
>+		p = np;
> 	}
> 	debug2("%s: compat KEX proposal: %s", __func__, p);
> 	if (*p == '\0')
>-- 
>2.17.1
>
>
>From e7156fa766b4bd01b7f6b72c60b5b7c80f7d98fa Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Wed, 18 Jul 2018 15:15:41 +0200
>Subject: [PATCH 3/4] coverity: RESOURCE_LEAK (CWE-772)
>
>openssh-7.7p1/mux.c:1063: leaked_storage: Variable "chost" going out of scope leaks the storage it points to.
> 1061|
> 1062|   	/* reply is deferred, sent by mux_session_confirm */
> 1063|-> 	return 0;
> 1064|   }
> 1065|
>---
> mux.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/mux.c b/mux.c
>index 6394e3e1..143ee145 100644
>--- a/mux.c
>+++ b/mux.c
>@@ -1042,6 +1042,7 @@ process_mux_stdio_fwd(struct ssh *ssh, u_int rid,
> 		set_nonblock(new_fd[1]);
> 
> 	nc = channel_connect_stdio_fwd(ssh, chost, cport, new_fd[0], new_fd[1]);
>+	free(chost);
> 
> 	nc->ctl_chan = c->self;		/* link session -> control channel */
> 	c->remote_id = nc->self; 	/* link control -> session channel */
>-- 
>2.17.1
>
>
>From ab015ffbd89b39aee7087c2a4e046ee68d938fb0 Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Wed, 18 Jul 2018 15:19:23 +0200
>Subject: [PATCH 4/4] sftp-client: RESOURCE_LEAK (CWE-772)
>
>openssh-7.7p1/sftp-client.c:1504: overwrite_var: Overwriting "new_dst" in "new_dst = path_append(dst, filename)" leaks the storage that "new_dst" points to.
> 1502|   		filename = dir_entries[i]->filename;
> 1503|
> 1504|-> 		new_dst = path_append(dst, filename);
> 1505|   		new_src = path_append(src, filename);
> 1506|
>---
> sftp-client.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
>diff --git a/sftp-client.c b/sftp-client.c
>index be4fd621..0e0a5d21 100644
>--- a/sftp-client.c
>+++ b/sftp-client.c
>@@ -1501,13 +1501,16 @@ download_dir_internal(struct sftp_conn *conn, const char *src, const char *dst,
> 	for (i = 0; dir_entries[i] != NULL && !interrupted; i++) {
> 		filename = dir_entries[i]->filename;
> 
>+		/* skip special directories */
>+		if (S_ISDIR(dir_entries[i]->a.perm))
>+			if (strcmp(filename, ".") == 0 ||
>+			    strcmp(filename, "..") == 0)
>+				continue;
>+
> 		new_dst = path_append(dst, filename);
> 		new_src = path_append(src, filename);
> 
> 		if (S_ISDIR(dir_entries[i]->a.perm)) {
>-			if (strcmp(filename, ".") == 0 ||
>-			    strcmp(filename, "..") == 0)
>-				continue;
> 			if (download_dir_internal(conn, new_src, new_dst,
> 			    depth + 1, &(dir_entries[i]->a), preserve_flag,
> 			    print_flag, resume_flag, fsync_flag) == -1)
>@@ -1842,9 +1845,15 @@ upload_dir_internal(struct sftp_conn *conn, const char *src, const char *dst,
> 	}
> 
> 	while (((dp = readdir(dirp)) != NULL) && !interrupted) {
>+		filename = dp->d_name;
>+
> 		if (dp->d_ino == 0)
> 			continue;
>-		filename = dp->d_name;
>+		if (S_ISDIR(sb.st_mode))
>+			if (strcmp(filename, ".") == 0 ||
>+			    strcmp(filename, "..") == 0)
>+				continue;
>+
> 		new_dst = path_append(dst, filename);
> 		new_src = path_append(src, filename);
> 
>@@ -1853,10 +1862,6 @@ upload_dir_internal(struct sftp_conn *conn, const char *src, const char *dst,
> 			    strerror(errno));
> 			ret = -1;
> 		} else if (S_ISDIR(sb.st_mode)) {
>-			if (strcmp(filename, ".") == 0 ||
>-			    strcmp(filename, "..") == 0)
>-				continue;
>-
> 			if (upload_dir_internal(conn, new_src, new_dst,
> 			    depth + 1, preserve_flag, print_flag, resume,
> 			    fsync_flag) == -1)
>-- 
>2.17.1
>

Actions: View

Attachments on bug 2883: 3164 | 3166