From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 16:06:16 +0100
Some update suggestions were taken into account
from static source code analysis.
Markus Elfring (9):
Delete unnecessary code in user_init_raw_fds()
Less checks in user_init_raw_fds() after error detection
Adjust an error message in user_init_socket_fds()
Delete an unnecessary check before kfree() in user_init_socket_fds()
Delete two unnecessary checks before freeaddrinfo() in user_init_socket_fds()
Less checks in user_init_socket_fds() after error detection
Adjust an error message in user_init_tap_fds()
Less checks in user_init_tap_fds() after error detection
Delete an unnecessary variable initialisation in user_init_tap_fds()
arch/um/drivers/vector_user.c | 133 +++++++++++++++++++-----------------------
1 file changed, 60 insertions(+), 73 deletions(-)
--
2.16.2
From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 11:36:18 +0100
* One condition check could never be reached with a non-null pointer
at the end of this function. Thus remove the corresponding statement.
* Delete an initialisation for the local variable "result"
which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/um/drivers/vector_user.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 4291f1a5d342..d6a6207d4061 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -211,7 +211,7 @@ static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)
struct sockaddr_ll sock;
int err = -ENOMEM;
char *iface;
- struct vector_fds *result = NULL;
+ struct vector_fds *result;
int optval = 1;
@@ -276,8 +276,6 @@ static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)
os_close_file(rxfd);
if (txfd >= 0)
os_close_file(txfd);
- if (result != NULL)
- kfree(result);
return NULL;
}
--
2.16.2
From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 11:40:14 +0100
Up to two checks could be repeated by the user_init_raw_fds() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.
* Adjust jump targets so that an extra check can be omitted at the end.
* Delete an initialisation for the local variable "rxfd"
which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/um/drivers/vector_user.c | 61 ++++++++++++++++++++-----------------------
1 file changed, 28 insertions(+), 33 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index d6a6207d4061..037cd85ee424 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -207,54 +207,46 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)
{
struct ifreq ifr;
- int rxfd = -1, txfd = -1;
+ int rxfd, txfd = -1;
struct sockaddr_ll sock;
- int err = -ENOMEM;
- char *iface;
struct vector_fds *result;
int optval = 1;
+ int err;
+ char *iface = uml_vector_fetch_arg(ifspec, TOKEN_IFNAME);
-
- iface = uml_vector_fetch_arg(ifspec, TOKEN_IFNAME);
- if (iface == NULL)
- goto cleanup;
+ if (!iface) {
+ err = -ENOMEM;
+ goto report_failure;
+ }
rxfd = socket(AF_PACKET, SOCK_RAW, ETH_P_ALL);
- if (rxfd == -1) {
- err = -errno;
- goto cleanup;
- }
+ if (rxfd == -1)
+ goto set_error_code;
+
txfd = socket(AF_PACKET, SOCK_RAW, 0); /* Turn off RX on this fd */
- if (txfd == -1) {
- err = -errno;
- goto cleanup;
- }
+ if (txfd == -1)
+ goto set_error_code;
+
memset(&ifr, 0, sizeof(ifr));
strncpy((char *)&ifr.ifr_name, iface, sizeof(ifr.ifr_name) - 1);
- if (ioctl(rxfd, SIOCGIFINDEX, (void *) &ifr) < 0) {
- err = -errno;
- goto cleanup;
- }
+ if (ioctl(rxfd, SIOCGIFINDEX, (void *) &ifr) < 0)
+ goto set_error_code;
sock.sll_family = AF_PACKET;
sock.sll_protocol = htons(ETH_P_ALL);
sock.sll_ifindex = ifr.ifr_ifindex;
- if (bind(rxfd,
- (struct sockaddr *) &sock, sizeof(struct sockaddr_ll)) < 0) {
- err = -errno;
- goto cleanup;
- }
+ if (bind(rxfd, (struct sockaddr *)&sock, sizeof(struct sockaddr_ll))
+ < 0)
+ goto set_error_code;
sock.sll_family = AF_PACKET;
sock.sll_protocol = htons(ETH_P_IP);
sock.sll_ifindex = ifr.ifr_ifindex;
- if (bind(txfd,
- (struct sockaddr *) &sock, sizeof(struct sockaddr_ll)) < 0) {
- err = -errno;
- goto cleanup;
- }
+ if (bind(txfd, (struct sockaddr *)&sock, sizeof(struct sockaddr_ll))
+ < 0)
+ goto set_error_code;
if (setsockopt(txfd,
SOL_PACKET, PACKET_QDISC_BYPASS,
@@ -270,12 +262,15 @@ static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)
result->remote_addr_size = 0;
}
return result;
-cleanup:
- printk(UM_KERN_ERR "user_init_raw: init failed, error %d", err);
- if (rxfd >= 0)
- os_close_file(rxfd);
+
+set_error_code:
+ err = -errno;
+ os_close_file(rxfd);
+
if (txfd >= 0)
os_close_file(txfd);
+report_failure:
+ printk(UM_KERN_ERR "%s: init failed: %d", __func__, err);
return NULL;
}
--
2.16.2
From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 13:53:08 +0100
* Adjust an error message at the end of this function.
* Delete the local variable "err" which became unnecessary
with this refactoring.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/um/drivers/vector_user.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 037cd85ee424..9cc4651990e3 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -305,7 +305,6 @@ bool uml_tap_enable_vnet_headers(int fd)
static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)
{
- int err = -ENOMEM;
int fd = -1, gairet;
struct addrinfo srchints;
struct addrinfo dsthints;
@@ -419,7 +418,7 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)
cleanup:
if (gairesult != NULL)
freeaddrinfo(gairesult);
- printk(UM_KERN_ERR "user_init_socket: init failed, error %d", err);
+
if (fd >= 0)
os_close_file(fd);
if (result != NULL) {
@@ -427,6 +426,8 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)
kfree(result->remote_addr);
kfree(result);
}
+
+ printk(UM_KERN_ERR "%s: init failed: %d", __func__, -ENOMEM);
return NULL;
}
--
2.16.2
From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 14:00:09 +0100
The kfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/um/drivers/vector_user.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 9cc4651990e3..e831bd85cad4 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -422,8 +422,7 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)
if (fd >= 0)
os_close_file(fd);
if (result != NULL) {
- if (result->remote_addr != NULL)
- kfree(result->remote_addr);
+ kfree(result->remote_addr);
kfree(result);
}
--
2.16.2
From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 14:20:46 +0100
The implementation returns from this function if a null pointer
was detected in the local variable "gairesult". Thus the check
before two calls of the function "freeaddrinfo" is not needed.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/um/drivers/vector_user.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index e831bd85cad4..2dee1e183387 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -384,9 +384,7 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)
goto cleanup;
}
- if (gairesult != NULL)
- freeaddrinfo(gairesult);
-
+ freeaddrinfo(gairesult);
gairesult = NULL;
gairet = getaddrinfo(dst, dstport, &dsthints, &gairesult);
@@ -416,8 +414,7 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)
freeaddrinfo(gairesult);
return result;
cleanup:
- if (gairesult != NULL)
- freeaddrinfo(gairesult);
+ freeaddrinfo(gairesult);
if (fd >= 0)
os_close_file(fd);
--
2.16.2
From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 14:56:38 +0100
Two checks could be repeated by the user_init_socket_fds() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.
* Adjust jump targets.
* Delete two sanity checks and a call of the function "kfree"
which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/um/drivers/vector_user.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 2dee1e183387..4c265262a369 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -375,13 +375,13 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)
"socket_open : could not open socket, error = %d",
-errno
);
- goto cleanup;
+ goto free_info;
}
if (bind(fd,
(struct sockaddr *) gairesult->ai_addr,
gairesult->ai_addrlen)) {
printk(UM_KERN_ERR L2TPV3_BIND_FAIL, errno);
- goto cleanup;
+ goto close_file;
}
freeaddrinfo(gairesult);
@@ -403,7 +403,8 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)
result->remote_addr = uml_kmalloc(
gairesult->ai_addrlen, UM_GFP_KERNEL);
if (result->remote_addr == NULL)
- goto cleanup;
+ goto free_result;
+
result->remote_addr_size = gairesult->ai_addrlen;
memcpy(
result->remote_addr,
@@ -413,16 +414,13 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)
}
freeaddrinfo(gairesult);
return result;
-cleanup:
- freeaddrinfo(gairesult);
-
- if (fd >= 0)
- os_close_file(fd);
- if (result != NULL) {
- kfree(result->remote_addr);
- kfree(result);
- }
+free_result:
+ kfree(result);
+close_file:
+ os_close_file(fd);
+free_info:
+ freeaddrinfo(gairesult);
printk(UM_KERN_ERR "%s: init failed: %d", __func__, -ENOMEM);
return NULL;
}
--
2.16.2
From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 15:10:05 +0100
Adjust an error message at the end of this function so that its name
will be automatically determined as a parameter.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/um/drivers/vector_user.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 4c265262a369..5e78723c34d4 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -192,7 +192,6 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
}
return result;
tap_cleanup:
- printk(UM_KERN_ERR "user_init_tap: init failed, error %d", err);
if (result != NULL) {
if (result->rx_fd >= 0)
os_close_file(result->rx_fd);
@@ -200,6 +199,8 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
os_close_file(result->tx_fd);
kfree(result);
}
+
+ printk(UM_KERN_ERR "%s: init failed: %d", __func__, err);
return NULL;
}
--
2.16.2
From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 15:43:31 +0100
Three checks could be repeated by the user_init_tap_fds() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.
* Adjust jump targets.
* Delete three sanity checks and an initialisation (for the local
variable "result") which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/um/drivers/vector_user.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 5e78723c34d4..bd625034a0f0 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -123,18 +123,18 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
struct sockaddr_ll sock;
int err = -ENOMEM, offload;
char *iface;
- struct vector_fds *result = NULL;
+ struct vector_fds *result;
iface = uml_vector_fetch_arg(ifspec, TOKEN_IFNAME);
if (iface == NULL) {
printk(UM_KERN_ERR "uml_tap: failed to parse interface spec\n");
- goto tap_cleanup;
+ goto report_failure;
}
result = uml_kmalloc(sizeof(struct vector_fds), UM_GFP_KERNEL);
if (result == NULL) {
printk(UM_KERN_ERR "uml_tap: failed to allocate file descriptors\n");
- goto tap_cleanup;
+ goto report_failure;
}
result->rx_fd = -1;
result->tx_fd = -1;
@@ -146,7 +146,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
fd = open(PATH_NET_TUN, O_RDWR);
if (fd < 0) {
printk(UM_KERN_ERR "uml_tap: failed to open tun device\n");
- goto tap_cleanup;
+ goto free_result;
}
result->tx_fd = fd;
memset(&ifr, 0, sizeof(ifr));
@@ -156,7 +156,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
err = ioctl(fd, TUNSETIFF, (void *) &ifr);
if (err != 0) {
printk(UM_KERN_ERR "uml_tap: failed to select tap interface\n");
- goto tap_cleanup;
+ goto close_tx_fd;
}
offload = TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6;
@@ -168,7 +168,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
if (fd == -1) {
printk(UM_KERN_ERR
"uml_tap: failed to create socket: %i\n", -errno);
- goto tap_cleanup;
+ goto close_tx_fd;
}
result->rx_fd = fd;
memset(&ifr, 0, sizeof(ifr));
@@ -176,7 +176,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
if (ioctl(fd, SIOCGIFINDEX, (void *) &ifr) < 0) {
printk(UM_KERN_ERR
"uml_tap: failed to set interface: %i\n", -errno);
- goto tap_cleanup;
+ goto close_rx_fd;
}
sock.sll_family = AF_PACKET;
@@ -188,18 +188,17 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
printk(UM_KERN_ERR
"user_init_tap: failed to bind raw pair, err %d\n",
-errno);
- goto tap_cleanup;
+ goto close_rx_fd;
}
return result;
-tap_cleanup:
- if (result != NULL) {
- if (result->rx_fd >= 0)
- os_close_file(result->rx_fd);
- if (result->tx_fd >= 0)
- os_close_file(result->tx_fd);
- kfree(result);
- }
+close_rx_fd:
+ os_close_file(result->rx_fd);
+close_tx_fd:
+ os_close_file(result->tx_fd);
+free_result:
+ kfree(result);
+report_failure:
printk(UM_KERN_ERR "%s: init failed: %d", __func__, err);
return NULL;
}
--
2.16.2
From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 15:50:29 +0100
The local variable "fd" will eventually be set to an appropriate value
a bit later. Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/um/drivers/vector_user.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index bd625034a0f0..3bd510eded58 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -119,9 +119,8 @@ struct arglist *uml_parse_vector_ifspec(char *arg)
static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
{
struct ifreq ifr;
- int fd = -1;
struct sockaddr_ll sock;
- int err = -ENOMEM, offload;
+ int err = -ENOMEM, fd, offload;
char *iface;
struct vector_fds *result;
--
2.16.2
Thanks, well noted.
It still does not fix it completely though.
Re-reading the code it will leak a fd if the malloc for result fails.
That return result; there should be inside the conditional falling back
to cleanup if the alloc fails.
A.
On 03/11/18 15:16, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Sun, 11 Mar 2018 11:36:18 +0100
>
> * One condition check could never be reached with a non-null pointer
> at the end of this function. Thus remove the corresponding statement.
>
> * Delete an initialisation for the local variable "result"
> which became unnecessary with this refactoring.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> arch/um/drivers/vector_user.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
> index 4291f1a5d342..d6a6207d4061 100644
> --- a/arch/um/drivers/vector_user.c
> +++ b/arch/um/drivers/vector_user.c
> @@ -211,7 +211,7 @@ static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)
> struct sockaddr_ll sock;
> int err = -ENOMEM;
> char *iface;
> - struct vector_fds *result = NULL;
> + struct vector_fds *result;
> int optval = 1;
>
>
> @@ -276,8 +276,6 @@ static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)
> os_close_file(rxfd);
> if (txfd >= 0)
> os_close_file(txfd);
> - if (result != NULL)
> - kfree(result);
> return NULL;
> }
>
--
Anton R. Ivanov
Cambridge Greys Limited, England and Wales company No 10273661
http://www.cambridgegreys.com/
> +set_error_code:
> + err = -errno;
> + os_close_file(rxfd);
I have taken another look at this change idea.
Now I notice that I should have preserved a sanity check there.
if (rxfd >= 0)
os_close_file(rxfd);
Regards,
Markus
> Date: Sun, 11 Mar 2018 16:06:16 +0100
>
> Some update suggestions were taken into account
> from static source code analysis.
…
> Delete unnecessary code in user_init_raw_fds()
> Less checks in user_init_raw_fds() after error detection
> Adjust an error message in user_init_socket_fds()
> Delete an unnecessary check before kfree() in user_init_socket_fds()
> Delete two unnecessary checks before freeaddrinfo() in user_init_socket_fds()
> Less checks in user_init_socket_fds() after error detection
> Adjust an error message in user_init_tap_fds()
> Less checks in user_init_tap_fds() after error detection
> Delete an unnecessary variable initialisation in user_init_tap_fds()
…
Would you like to pick any idea up from this patch series?
Regards,
Markus