[PATCH iproute2] libnetlink: fix socket leak in rtnl_open_byproto()

[PATCH iproute2] libnetlink: fix socket leak in rtnl_open_byproto()

[Maxim Petrov]

rtnl_open_byproto() does not close the opened socket in case of errors, and the
socket is returned to the caller in the `fd` field of the struct. However, none
of the callers care about the socket, so close it in the function immediately to
avoid any potential resource leaks.

Signed-off-by: Maxim Petrov <mmrmaximuzz@gmail.com>
---
 lib/libnetlink.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 7e977a67..6d1b1187 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -210,13 +210,13 @@ int rtnl_open_byproto(struct rtnl_handle *rth, unsigned int subscriptions,
 	if (setsockopt(rth->fd, SOL_SOCKET, SO_SNDBUF,
 		       &sndbuf, sizeof(sndbuf)) < 0) {
 		perror("SO_SNDBUF");
-		return -1;
+		goto err;
 	}
 
 	if (setsockopt(rth->fd, SOL_SOCKET, SO_RCVBUF,
 		       &rcvbuf, sizeof(rcvbuf)) < 0) {
 		perror("SO_RCVBUF");
-		return -1;
+		goto err;
 	}
 
 	/* Older kernels may no support extended ACK reporting */
@@ -230,25 +230,28 @@ int rtnl_open_byproto(struct rtnl_handle *rth, unsigned int subscriptions,
 	if (bind(rth->fd, (struct sockaddr *)&rth->local,
 		 sizeof(rth->local)) < 0) {
 		perror("Cannot bind netlink socket");
-		return -1;
+		goto err;
 	}
 	addr_len = sizeof(rth->local);
 	if (getsockname(rth->fd, (struct sockaddr *)&rth->local,
 			&addr_len) < 0) {
 		perror("Cannot getsockname");
-		return -1;
+		goto err;
 	}
 	if (addr_len != sizeof(rth->local)) {
 		fprintf(stderr, "Wrong address length %d\n", addr_len);
-		return -1;
+		goto err;
 	}
 	if (rth->local.nl_family != AF_NETLINK) {
 		fprintf(stderr, "Wrong address family %d\n",
 			rth->local.nl_family);
-		return -1;
+		goto err;
 	}
 	rth->seq = time(NULL);
 	return 0;
+err:
+	rtnl_close(rth);
+	return -1;
 }
 
 int rtnl_open(struct rtnl_handle *rth, unsigned int subscriptions)
-- 
2.25.1

Re: [PATCH iproute2] libnetlink: fix socket leak in rtnl_open_byproto()

[Stephen Hemminger]

On Tue, 8 Feb 2022 20:20:45 +0300
Maxim Petrov <mmrmaximuzz@gmail.com> wrote:

> rtnl_open_byproto() does not close the opened socket in case of errors, and the
> socket is returned to the caller in the `fd` field of the struct. However, none
> of the callers care about the socket, so close it in the function immediately to
> avoid any potential resource leaks.
> 
> Signed-off-by: Maxim Petrov <mmrmaximuzz@gmail.com>

Can do the same thing without introducing a goto

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 7e977a6762f8..0ed6d68b5c08 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -226,29 +226,26 @@ int rtnl_open_byproto(struct rtnl_handle *rth, unsigned int subscriptions,
 	memset(&rth->local, 0, sizeof(rth->local));
 	rth->local.nl_family = AF_NETLINK;
 	rth->local.nl_groups = subscriptions;
+	addr_len = sizeof(rth->local);
 
 	if (bind(rth->fd, (struct sockaddr *)&rth->local,
 		 sizeof(rth->local)) < 0) {
 		perror("Cannot bind netlink socket");
-		return -1;
-	}
-	addr_len = sizeof(rth->local);
-	if (getsockname(rth->fd, (struct sockaddr *)&rth->local,
+	} else if (getsockname(rth->fd, (struct sockaddr *)&rth->local,
 			&addr_len) < 0) {
 		perror("Cannot getsockname");
-		return -1;
-	}
-	if (addr_len != sizeof(rth->local)) {
+	} else if (addr_len != sizeof(rth->local)) {
 		fprintf(stderr, "Wrong address length %d\n", addr_len);
-		return -1;
-	}
-	if (rth->local.nl_family != AF_NETLINK) {
+	} else if (rth->local.nl_family != AF_NETLINK) {
 		fprintf(stderr, "Wrong address family %d\n",
 			rth->local.nl_family);
-		return -1;
+	} else {
+		rth->seq = time(NULL);
+		return 0;
 	}
-	rth->seq = time(NULL);
-	return 0;
+
+	rtnl_close(rth);
+	return -1;
 }
 
 int rtnl_open(struct rtnl_handle *rth, unsigned int subscriptions)
-- 
2.34.1

Re: [PATCH iproute2] libnetlink: fix socket leak in rtnl_open_byproto()

[Maxim Petrov]

Hello Stephen!

On 2022-02-11 01:19 UTC, Stephen Hemminger wrote:
> +	} else {
> +		rth->seq = time(NULL);
> +		return 0;
>  	}

For me it looks slightly alien as the normal flow jumps from one 'else if' to
another, and the final return statement is hidden inside the else block. The
original version is straightforward and less surprising.

> Can do the same thing without introducing a goto
But what's wrong with the goto here? I thought it is a perfectly legal C way to
handle errors, and iproute2 uses it for that purpose almost everywhere.

Re: [PATCH iproute2] libnetlink: fix socket leak in rtnl_open_byproto()

[Stephen Hemminger]

On Fri, 11 Feb 2022 21:30:11 +0300
Maxim Petrov <mmrmaximuzz@gmail.com> wrote:

> Hello Stephen!
> 
> On 2022-02-11 01:19 UTC, Stephen Hemminger wrote:
> > +	} else {
> > +		rth->seq = time(NULL);
> > +		return 0;
> >  	}  
> 
> For me it looks slightly alien as the normal flow jumps from one 'else if' to
> another, and the final return statement is hidden inside the else block. The
> original version is straightforward and less surprising.
> 
> > Can do the same thing without introducing a goto  
> But what's wrong with the goto here? I thought it is a perfectly legal C way to
> handle errors, and iproute2 uses it for that purpose almost everywhere.

Ok, either way. personal preference only.

Re: [PATCH iproute2] libnetlink: fix socket leak in rtnl_open_byproto()

[Stephen Hemminger]

On Tue, 8 Feb 2022 20:20:45 +0300
Maxim Petrov <mmrmaximuzz@gmail.com> wrote:

> rtnl_open_byproto() does not close the opened socket in case of errors, and the
> socket is returned to the caller in the `fd` field of the struct. However, none
> of the callers care about the socket, so close it in the function immediately to
> avoid any potential resource leaks.
> 
> Signed-off-by: Maxim Petrov <mmrmaximuzz@gmail.com>

Applied with reindent of commit message.
Checkpatch was complaining about long lines.