Re: [PATCH] util: Fix return type mismatch for nl_recv

[Laine Stump <laine@xxxxxxxxx>]

On 05/12/2014 01:57 PM, Wangrui (K) wrote:

1.

As shown in the definition of nl_recv, its return value is of INT type.

 

int nl_recv(struct nl_handle *handle, struct sockaddr_nl *nla,

             unsigned char **buf, struct ucred **creds)

 

However, in function virNetlinkCommand, it uses an unsigned int param,

respbuflen, to receive its return value. Thus, the branch "*respbuflen < 0"

is unreachable, negative return values are handled incorrectly.

 

2.

It's said in nl_recv's description that "The caller is responsible for

freeing the buffer allocated * in \c *buf if a positive value is returned."

which means, we needn't to free it in case it fails.

Additionally, nl_recv has a BUG in handling buf: in the error branch, it frees

the buf, but didn't set it to NULL. Freeing it outside in the caller function

will cause double free.

Thus, we set but(resp) to NULL if nl_recv fails.

Thanks for finding this! It was a good catch, and your patch corrects the problem you describe, but I think maybe we should take this opportunity to clean up the function further:

1) the first error return from the function returns -EINVAL, implying that the return value will contain -errno, which is almost never the case.

2) for either of the first 2 error conditions, the function returns with garbage in *resp. for the next 5 (!) error conditions, *resp not only contains garbage, but we then "goto error" which attempts to VIR_FREE(*resp).

3) "rc" defaults to 0, and is set to -1 on error, while usually our code follows the style of calling it "ret", defaulting to -1, then setting to 0 just after the last opportunity for error.

4) as you pointed out in a separate patch, the code below the "error" label is not only executed when there is an error, but for all return paths, and in that case it's our convention to call it "cleanup".

I'm preparing a separate patch which will address all 4 of these problems. I'll Cc you when it is ready (should be within a few hours).

 

Signed-off-by: Zhang Bo <oscar.zhangbo@xxxxxxxxxx>

---

src/util/virnetlink.c | 8 ++++++--

1 file changed, 6 insertions(+), 2 deletions(-)

 

diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c

index 0cf18f2..1a09567 100644

--- a/src/util/virnetlink.c

+++ b/src/util/virnetlink.c

@@ -192,6 +192,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg,

     int n;

     struct nlmsghdr *nlmsg = nlmsg_hdr(nl_msg);

     virNetlinkHandle *nlhandle = NULL;

+    int length = 0;

     if (protocol >= MAX_LINKS) {

         virReportSystemError(EINVAL,

@@ -257,12 +258,15 @@ int virNetlinkCommand(struct nl_msg *nl_msg,

         goto error;

     }

-    *respbuflen = nl_recv(nlhandle, &nladdr,

-                          (unsigned char **)resp, NULL);

+    length = nl_recv(nlhandle, &nladdr,

+                     (unsigned char **)resp, NULL);

 

-    if (*respbuflen <= 0) {

+    if (length <= 0) {

         virReportSystemError(errno,

                              "%s", _("nl_recv failed"));

+       *resp = NULL;

         rc = -1;

+    } else {

+        *respbuflen = length;

     }

  error:

     if (rc == -1) {

--

1.7.12.4

 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list