dhcpcd-discuss

Dear Roy Marples san,

Nice to meet you via e-mail.
I am Koichi Okamoto, senior system engineer, to work for SONY Video & Sound Products Inc. in Tokyo.

Thank you for your dhcpcd open source software and for your maintenance continuously!

I'd like to inform you of my patch to fix some resources leak issues.

If you can agree to these solutions, I hope you could apply this patch for dhcpcd main branch.

Please note that these patches are created for dhcpcd-6.9.2 version (older version, sorry).
Only "0006-A-resource-leak-issue-is-fixed.patch" may be able to use for latest version.

I wish this patch may contribute a little for your dhcpcd open source software confidence.

Could you please see the purpose of these six patches one by one as follows:

# 0001-double-free-possibility-is-fixed.patch
From 9c00caafa0a124a04df0534f57e8e9bf0559a0b4 Mon Sep 17 00:00:00 2001
From: Koichi Okamoto <Koichi.Okamoto@xxxxxxxxxxx>
Date: Mon, 14 Dec 2015 15:28:13 +0900
Subject: [PATCH 1/6] double free possibility is fixed

dhcp_arp_conflicted in dhcp.c has the possibility to access
struct arp_state* astate argument after freed this argument.

arp_free(astate) means they don't need to access this pointer
anymore. In addition, if arp_free(astate) carry out in first
if sentence and then arp_free(astate) performes in second if
sentence in case of "amsg != NULL", astate double free will
happen. Therefore I add "return;" for each
set of arp_free(astate) sentence at the last position.
---
 dhcp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/dhcp.c b/dhcp.c
index a77e442..7866baa 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -2000,6 +2000,7 @@ dhcp_arp_conflicted(struct arp_state *astate, const struct arp_msg *amsg)
 		arp_free(astate);
 		eloop_timeout_delete(ifp->ctx->eloop, NULL, ifp);
 		dhcpcd_startinterface(ifp);
+		return;
 	}
 
 	/* RFC 2131 3.1.5, Client-server interaction
@@ -2030,6 +2031,7 @@ dhcp_arp_conflicted(struct arp_state *astate, const struct arp_msg *amsg)
 		eloop_timeout_delete(ifp->ctx->eloop, NULL, ifp);
 		eloop_timeout_add_sec(ifp->ctx->eloop,
 		    DHCP_RAND_MAX, dhcp_discover, ifp);
+		return;
 	}
 }
 
-- 
1.9.1


# 0002-A-resource-leak-issue-is-fixed.patch
From 7802681e1a575e297e8c6ae98c26c1020167680f Mon Sep 17 00:00:00 2001
From: Koichi Okamoto <Koichi.Okamoto@xxxxxxxxxxx>
Date: Mon, 14 Dec 2015 20:38:11 +0900
Subject: [PATCH 2/6] A resource leak issue is fixed

send_interface1 in script.c has a resource leaks issue
for char **env pointer which is alocated by make_env function when
arraytostr fucntion retruns "-1" value. In this case, these
resource is also freed.
---
 script.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/script.c b/script.c
index 4f6a6cc..3264aa8 100644
--- a/script.c
+++ b/script.c
@@ -571,9 +571,10 @@ send_interface1(struct fd_list *fd, const struct interface *iface,
 	elen = (size_t)arraytostr((const char *const *)env, &s);
 	if ((ssize_t)elen == -1) {
 		free(s);
-		return -1;
+		retval = -1;
+	} else {
+		retval = control_queue(fd, s, elen, 1);
 	}
-	retval = control_queue(fd, s, elen, 1);
 	ep = env;
 	while (*ep)
 		free(*ep++);
-- 
1.9.1


# 0003-A-resource-leak-issue-is-fixed.patch
From 7bc0cd1bf7d00ef9bbae3f75576ffe52257fcb8b Mon Sep 17 00:00:00 2001
From: Koichi Okamoto <Koichi.Okamoto@xxxxxxxxxxx>
Date: Tue, 22 Dec 2015 09:36:40 +0900
Subject: [PATCH 3/6] A resource leak issue is fixed

dhcp6_readlease function in dhcp6.c has a resource leak issue
for handle of lease file in case of malloc fail. In this case,
close(fd) is added to close file handle.
---
 dhcp6.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/dhcp6.c b/dhcp6.c
index 3e33e4f..3cf0f86 100644
--- a/dhcp6.c
+++ b/dhcp6.c
@@ -2117,8 +2117,10 @@ dhcp6_readlease(struct interface *ifp, int validate)
 	}
 	if ((fd = open(state->leasefile, O_RDONLY)) == -1)
 		return -1;
-	if ((state->new = malloc((size_t)st.st_size)) == NULL)
+	if ((state->new = malloc((size_t)st.st_size)) == NULL) {
+		close(fd);
 		return -1;
+	}
 	retval = -1;
 	state->new_len = (size_t)st.st_size;
 	bytes = read(fd, state->new, state->new_len);
-- 
1.9.1


# 0004-A-resource-leak-issue-is-fixed.patch
From 50db663950f810fd1b6caa27f61492ebcb646403 Mon Sep 17 00:00:00 2001
From: Koichi Okamoto <Koichi.Okamoto@xxxxxxxxxxx>
Date: Tue, 22 Dec 2015 17:40:28 +0900
Subject: [PATCH 4/6] A resource leak issue is fixed

read_config function in if-options.c has a resource leak issue
for struct if_options *ifo memory when malloc is fail at L2188.
Before exiting this function in case of malloc fail,
free_options(ifo) is invoked to free *ifo memory.
---
 if-options.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/if-options.c b/if-options.c
index e998617..29c6cae 100644
--- a/if-options.c
+++ b/if-options.c
@@ -2188,6 +2188,7 @@ read_config(struct dhcpcd_ctx *ctx,
 		buf = malloc(buflen);
 		if (buf == NULL) {
 			logger(ctx, LOG_ERR, "%s: %m", __func__);
+			free_options(ifo);
 			return NULL;
 		}
 		ldop = edop = NULL;
-- 
1.9.1


# 0005-A-resource-leak-issue-is-fixed.patch
From 6c433d628160e114c4a39ef592c936daf75f89b0 Mon Sep 17 00:00:00 2001
From: Koichi Okamoto <Koichi.Okamoto@xxxxxxxxxxx>
Date: Tue, 22 Dec 2015 20:32:30 +0900
Subject: [PATCH 5/6] A resource leak issue is fixed

parse_option function in if-options.c has a resource leak issue
at L1126 realloc in case of fail for the memory to be created
by strdup function. In failure case, this memory is freed.
---
 if-options.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/if-options.c b/if-options.c
index 29c6cae..613f805 100644
--- a/if-options.c
+++ b/if-options.c
@@ -1126,6 +1126,7 @@ parse_option(struct dhcpcd_ctx *ctx, const char *ifname, struct if_options *ifo,
 			nconf = realloc(ifo->config, sizeof(char *) * (dl + 2));
 			if (nconf == NULL) {
 				logger(ctx, LOG_ERR, "%s: %m", __func__);
+				free(p);
 				return -1;
 			}
 			ifo->config = nconf;
-- 
1.9.1


# 0006-A-resource-leak-issue-is-fixed.patch
From c9f4991d12eb1370ccb4515d15a70a1a6397f572 Mon Sep 17 00:00:00 2001
From: Koichi Okamoto <Koichi.Okamoto@xxxxxxxxxxx>
Date: Wed, 23 Dec 2015 13:00:50 +0900
Subject: [PATCH 6/6] A resource leak issue is fixed

ipv6_readsecret function in ipv6.c has a resource leak issue
for FILE handle which is located in /data/dhcpcd/etc directory.

In case of chmod fail, FILE handle cannot be closed.
As we don't use SLAAC private currently, this function isn't
invoked. But we will use it when Linux Kernel version will be 3.17
or later. Hence, I fix this issue.
---
 ipv6.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/ipv6.c b/ipv6.c
index e5ab9e5..4d90462 100644
--- a/ipv6.c
+++ b/ipv6.c
@@ -246,9 +246,12 @@ ipv6_readsecret(struct dhcpcd_ctx *ctx)
 	/* Ensure that only the dhcpcd user can read the secret.
 	 * Write permission is also denied as chaning it would remove
 	 * it's stability. */
-	if ((fp = fopen(SECRET, "w")) == NULL ||
-	    chmod(SECRET, S_IRUSR) == -1)
+	if ((fp = fopen(SECRET, "w")) == NULL) {
 		goto eexit;
+	} else if(chmod(SECRET, S_IRUSR) == -1) {
+		fclose(fp);
+		goto eexit;
+	}
 	x = fprintf(fp, "%s\n",
 	    hwaddr_ntoa(ctx->secret, ctx->secret_len, line, sizeof(line)));
 	if (fclose(fp) == EOF)
-- 
1.9.1


That's all.

Thank you for your confirmation and your efforts.

Best Regards,
Koichi Okamoto

Attachment: 0001-double-free-possibility-is-fixed.patch
Description: 0001-double-free-possibility-is-fixed.patch

Attachment: 0002-A-resource-leak-issue-is-fixed.patch
Description: 0002-A-resource-leak-issue-is-fixed.patch

Attachment: 0003-A-resource-leak-issue-is-fixed.patch
Description: 0003-A-resource-leak-issue-is-fixed.patch

Attachment: 0004-A-resource-leak-issue-is-fixed.patch
Description: 0004-A-resource-leak-issue-is-fixed.patch

Attachment: 0005-A-resource-leak-issue-is-fixed.patch
Description: 0005-A-resource-leak-issue-is-fixed.patch

Attachment: 0006-A-resource-leak-issue-is-fixed.patch
Description: 0006-A-resource-leak-issue-is-fixed.patch