[libvirt] [PATCH] util: refactor virNetlinkCommand to fix several bugs / style problems
Inspired by a simpler patch from "Wangrui (K) <moon.wangrui@huawei.com>". A submitted patch pointed out that virNetlinkCommand() was doing an improper typecast of the return value from nl_recv() (int to unsigned), causing it to miss error returns, and that even after remedying that problem, virNetlinkCommand() was calling VIR_FREE() on the pointer returned from nl_recv() (*resp) even if nl_recv() had returned an error, and that in this case the pointer was verifiably invalid, as it was pointing to memory that had been allocated by libnl, but then freed prior to returning the error. While reviewing this patch, I noticed several other problems with this seemingly simple function (at least one of them as serious as the problem being reported/fixed by the aforementioned patch), and decided they all deserved to be fixed. Here is the list: 1) The return value from nl_recv() must be assigned to an int (rather than unsigned int) in order to detect failure. 2) When nl_recv() returns an error, the contents of *resp is invalid, and should be simply set to 0, *not* VIR_FREE()'d. 3) The first error return from virNetlinkCommand returns -EINVAL, incorrectly implying that the caller can expect the return value to be of the "-errno" variety, which is not true in any other case. 4) The 2nd error return returns directly with garbage in *resp. While the caller should never use *resp in this case, it's still good practice to set it to NULL. 5) For the next 5 (!!) error conditions, *resp will contain garbage, and virNetlinkCommand() will goto it's cleanup code which will VIR_FREE(*resp), almost surely leading to a segfault. In addition to fixing these 5 problems, this patch also makes the following two changes to make the function conform more closely to the style of other libvirt code: 1) Change the handling of return code from "named rc and defaulted to 0, but changed to -1 on error" to the more common "named ret and defaulted to -1, but changed to 0 on success". 2) Rename the "error" label to "cleanup", since the code that follows is executed in success cases as well as failure. --- src/util/virnetlink.c | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 0cf18f2..db8d59e 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * Copyright (C) 2010-2012 IBM Corporation * * This library is free software; you can redistribute it and/or @@ -180,7 +180,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, uint32_t src_pid, uint32_t dst_pid, unsigned int protocol, unsigned int groups) { - int rc = 0; + int ret = -1; struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK, .nl_pid = dst_pid, @@ -192,41 +192,39 @@ int virNetlinkCommand(struct nl_msg *nl_msg, int n; struct nlmsghdr *nlmsg = nlmsg_hdr(nl_msg); virNetlinkHandle *nlhandle = NULL; + int len = 0; if (protocol >= MAX_LINKS) { virReportSystemError(EINVAL, _("invalid protocol argument: %d"), protocol); - return -EINVAL; + goto cleanup; } nlhandle = virNetlinkAlloc(); if (!nlhandle) { virReportSystemError(errno, "%s", _("cannot allocate nlhandle for netlink")); - return -1; + goto cleanup; } if (nl_connect(nlhandle, protocol) < 0) { virReportSystemError(errno, _("cannot connect to netlink socket with protocol %d"), protocol); - rc = -1; - goto error; + goto cleanup; } fd = nl_socket_get_fd(nlhandle); if (fd < 0) { virReportSystemError(errno, "%s", _("cannot get netlink socket fd")); - rc = -1; - goto error; + goto cleanup; } if (groups && nl_socket_add_membership(nlhandle, groups) < 0) { virReportSystemError(errno, "%s", _("cannot add netlink membership")); - rc = -1; - goto error; + goto cleanup; } nlmsg_set_dst(nl_msg, &nladdr); @@ -237,8 +235,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, if (nbytes < 0) { virReportSystemError(errno, "%s", _("cannot send to netlink socket")); - rc = -1; - goto error; + goto cleanup; } memset(fds, 0, sizeof(fds)); @@ -253,26 +250,27 @@ int virNetlinkCommand(struct nl_msg *nl_msg, if (n == 0) virReportSystemError(ETIMEDOUT, "%s", _("no valid netlink response was received")); - rc = -1; - goto error; + goto cleanup; } - *respbuflen = nl_recv(nlhandle, &nladdr, - (unsigned char **)resp, NULL); - if (*respbuflen <= 0) { + len = nl_recv(nlhandle, &nladdr, (unsigned char **)resp, NULL); + if (len <= 0) { virReportSystemError(errno, "%s", _("nl_recv failed")); - rc = -1; + goto cleanup; } - error: - if (rc == -1) { - VIR_FREE(*resp); + + ret = 0; + cleanup: + if (ret < 0) { *resp = NULL; *respbuflen = 0; + } else { + *respbuflen = len; } virNetlinkFree(nlhandle); - return rc; + return ret; } static void -- 1.9.0
Good improvement especially in coding style from which I learned difference between rc and ret. :)
-----Original Message----- From: Laine Stump [mailto:laine@laine.org] Sent: Tuesday, May 13, 2014 7:52 PM To: libvir-list@redhat.com; Wangrui (K); Yanqiangjun; Zhangbo (Oscar) Subject: [PATCH] util: refactor virNetlinkCommand to fix several bugs / style problems
Inspired by a simpler patch from "Wangrui (K) <moon.wangrui@huawei.com>".
A submitted patch pointed out that virNetlinkCommand() was doing an improper typecast of the return value from nl_recv() (int to unsigned), causing it to miss error returns, and that even after remedying that problem, virNetlinkCommand() was calling VIR_FREE() on the pointer returned from nl_recv() (*resp) even if nl_recv() had returned an error, and that in this case the pointer was verifiably invalid, as it was pointing to memory that had been allocated by libnl, but then freed prior to returning the error.
While reviewing this patch, I noticed several other problems with this seemingly simple function (at least one of them as serious as the problem being reported/fixed by the aforementioned patch), and decided they all deserved to be fixed. Here is the list:
1) The return value from nl_recv() must be assigned to an int (rather than unsigned int) in order to detect failure.
2) When nl_recv() returns an error, the contents of *resp is invalid, and should be simply set to 0, *not* VIR_FREE()'d.
3) The first error return from virNetlinkCommand returns -EINVAL, incorrectly implying that the caller can expect the return value to be of the "-errno" variety, which is not true in any other case.
4) The 2nd error return returns directly with garbage in *resp. While the caller should never use *resp in this case, it's still good practice to set it to NULL.
5) For the next 5 (!!) error conditions, *resp will contain garbage, and virNetlinkCommand() will goto it's cleanup code which will VIR_FREE(*resp), almost surely leading to a segfault.
In addition to fixing these 5 problems, this patch also makes the following two changes to make the function conform more closely to the style of other libvirt code:
1) Change the handling of return code from "named rc and defaulted to 0, but changed to -1 on error" to the more common "named ret and defaulted to -1, but changed to 0 on success".
2) Rename the "error" label to "cleanup", since the code that follows is executed in success cases as well as failure. --- src/util/virnetlink.c | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 0cf18f2..db8d59e 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * Copyright (C) 2010-2012 IBM Corporation * * This library is free software; you can redistribute it and/or @@ -180,7 +180,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, uint32_t src_pid, uint32_t dst_pid, unsigned int protocol, unsigned int groups) { - int rc = 0; + int ret = -1; struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK, .nl_pid = dst_pid, @@ -192,41 +192,39 @@ int virNetlinkCommand(struct nl_msg *nl_msg, int n; struct nlmsghdr *nlmsg = nlmsg_hdr(nl_msg); virNetlinkHandle *nlhandle = NULL; + int len = 0;
if (protocol >= MAX_LINKS) { virReportSystemError(EINVAL, _("invalid protocol argument: %d"), protocol); - return -EINVAL; + goto cleanup; }
nlhandle = virNetlinkAlloc(); if (!nlhandle) { virReportSystemError(errno, "%s", _("cannot allocate nlhandle for netlink")); - return -1; + goto cleanup; }
if (nl_connect(nlhandle, protocol) < 0) { virReportSystemError(errno, _("cannot connect to netlink socket with protocol %d"), protocol); - rc = -1; - goto error; + goto cleanup; }
fd = nl_socket_get_fd(nlhandle); if (fd < 0) { virReportSystemError(errno, "%s", _("cannot get netlink socket fd")); - rc = -1; - goto error; + goto cleanup; }
if (groups && nl_socket_add_membership(nlhandle, groups) < 0) { virReportSystemError(errno, "%s", _("cannot add netlink membership")); - rc = -1; - goto error; + goto cleanup; }
nlmsg_set_dst(nl_msg, &nladdr); @@ -237,8 +235,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, if (nbytes < 0) { virReportSystemError(errno, "%s", _("cannot send to netlink socket")); - rc = -1; - goto error; + goto cleanup; }
memset(fds, 0, sizeof(fds)); @@ -253,26 +250,27 @@ int virNetlinkCommand(struct nl_msg *nl_msg, if (n == 0) virReportSystemError(ETIMEDOUT, "%s", _("no valid netlink response was received")); - rc = -1; - goto error; + goto cleanup; }
- *respbuflen = nl_recv(nlhandle, &nladdr, - (unsigned char **)resp, NULL); - if (*respbuflen <= 0) { + len = nl_recv(nlhandle, &nladdr, (unsigned char **)resp, NULL); + if (len <= 0) { virReportSystemError(errno, "%s", _("nl_recv failed")); - rc = -1; + goto cleanup; } - error: - if (rc == -1) { - VIR_FREE(*resp); + + ret = 0; + cleanup: + if (ret < 0) { *resp = NULL; *respbuflen = 0; + } else { + *respbuflen = len; }
virNetlinkFree(nlhandle); - return rc; + return ret; }
static void -- 1.9.0
On 05/13/2014 01:51 PM, Laine Stump wrote:
Inspired by a simpler patch from "Wangrui (K) <moon.wangrui@huawei.com>".
A submitted patch pointed out that virNetlinkCommand() was doing an improper typecast of the return value from nl_recv() (int to unsigned), causing it to miss error returns, and that even after remedying that problem, virNetlinkCommand() was calling VIR_FREE() on the pointer returned from nl_recv() (*resp) even if nl_recv() had returned an error, and that in this case the pointer was verifiably invalid, as it was pointing to memory that had been allocated by libnl, but then freed prior to returning the error.
While reviewing this patch, I noticed several other problems with this seemingly simple function (at least one of them as serious as the problem being reported/fixed by the aforementioned patch), and decided they all deserved to be fixed. Here is the list:
1) The return value from nl_recv() must be assigned to an int (rather than unsigned int) in order to detect failure.
2) When nl_recv() returns an error, the contents of *resp is invalid, and should be simply set to 0, *not* VIR_FREE()'d.
3) The first error return from virNetlinkCommand returns -EINVAL, incorrectly implying that the caller can expect the return value to be of the "-errno" variety, which is not true in any other case.
4) The 2nd error return returns directly with garbage in *resp. While the caller should never use *resp in this case, it's still good practice to set it to NULL.
5) For the next 5 (!!) error conditions, *resp will contain garbage, and virNetlinkCommand() will goto it's cleanup code which will VIR_FREE(*resp), almost surely leading to a segfault.
In addition to fixing these 5 problems, this patch also makes the following two changes to make the function conform more closely to the style of other libvirt code:
1) Change the handling of return code from "named rc and defaulted to 0, but changed to -1 on error" to the more common "named ret and defaulted to -1, but changed to 0 on success".
2) Rename the "error" label to "cleanup", since the code that follows is executed in success cases as well as failure. --- src/util/virnetlink.c | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-)
ACK. Just nits below.
@@ -253,26 +250,27 @@ int virNetlinkCommand(struct nl_msg *nl_msg, if (n == 0) virReportSystemError(ETIMEDOUT, "%s", _("no valid netlink response was received")); - rc = -1; - goto error; + goto cleanup; }
- *respbuflen = nl_recv(nlhandle, &nladdr, - (unsigned char **)resp, NULL); - if (*respbuflen <= 0) { + len = nl_recv(nlhandle, &nladdr, (unsigned char **)resp, NULL); + if (len <= 0) {
Does nl_recv set errno when it returns 0?
virReportSystemError(errno, "%s", _("nl_recv failed")); - rc = -1; + goto cleanup; } - error: - if (rc == -1) { - VIR_FREE(*resp); +
[1] here more.
+ ret = 0; + cleanup: + if (ret < 0) { *resp = NULL; *respbuflen = 0; + } else { + *respbuflen = len;
Personally, I'd like this assignment [1]
}
virNetlinkFree(nlhandle); - return rc; + return ret; }
static void
Jan
On 05/16/2014 06:24 PM, Ján Tomko wrote:
On 05/13/2014 01:51 PM, Laine Stump wrote:
Inspired by a simpler patch from "Wangrui (K) <moon.wangrui@huawei.com>".
A submitted patch pointed out that virNetlinkCommand() was doing an improper typecast of the return value from nl_recv() (int to unsigned), causing it to miss error returns, and that even after remedying that problem, virNetlinkCommand() was calling VIR_FREE() on the pointer returned from nl_recv() (*resp) even if nl_recv() had returned an error, and that in this case the pointer was verifiably invalid, as it was pointing to memory that had been allocated by libnl, but then freed prior to returning the error.
While reviewing this patch, I noticed several other problems with this seemingly simple function (at least one of them as serious as the problem being reported/fixed by the aforementioned patch), and decided they all deserved to be fixed. Here is the list:
1) The return value from nl_recv() must be assigned to an int (rather than unsigned int) in order to detect failure.
2) When nl_recv() returns an error, the contents of *resp is invalid, and should be simply set to 0, *not* VIR_FREE()'d.
3) The first error return from virNetlinkCommand returns -EINVAL, incorrectly implying that the caller can expect the return value to be of the "-errno" variety, which is not true in any other case.
4) The 2nd error return returns directly with garbage in *resp. While the caller should never use *resp in this case, it's still good practice to set it to NULL.
5) For the next 5 (!!) error conditions, *resp will contain garbage, and virNetlinkCommand() will goto it's cleanup code which will VIR_FREE(*resp), almost surely leading to a segfault.
In addition to fixing these 5 problems, this patch also makes the following two changes to make the function conform more closely to the style of other libvirt code:
1) Change the handling of return code from "named rc and defaulted to 0, but changed to -1 on error" to the more common "named ret and defaulted to -1, but changed to 0 on success".
2) Rename the "error" label to "cleanup", since the code that follows is executed in success cases as well as failure. --- src/util/virnetlink.c | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-) ACK. Just nits below.
@@ -253,26 +250,27 @@ int virNetlinkCommand(struct nl_msg *nl_msg, if (n == 0) virReportSystemError(ETIMEDOUT, "%s", _("no valid netlink response was received")); - rc = -1; - goto error; + goto cleanup; }
- *respbuflen = nl_recv(nlhandle, &nladdr, - (unsigned char **)resp, NULL); - if (*respbuflen <= 0) { + len = nl_recv(nlhandle, &nladdr, (unsigned char **)resp, NULL); + if (len <= 0) { Does nl_recv set errno when it returns 0?
Good point - yet another existing bug that can be fixed in this patch. I'll add an "if (len == 0)" clause that prints out a plain error (since for us receiving nothing *is* an error).
virReportSystemError(errno, "%s", _("nl_recv failed")); - rc = -1; + goto cleanup; } - error: - if (rc == -1) { - VIR_FREE(*resp); +
[1] here more.
+ ret = 0; + cleanup: + if (ret < 0) { *resp = NULL; *respbuflen = 0; + } else { + *respbuflen = len; Personally, I'd like this assignment [1]
Sure, I can do that. I pushed the patch with those two changes. Thanks for the attentive review!
participants (3)
-
Ján Tomko -
Laine Stump -
Wangrui (K)