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

[Laine Stump]

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 at huawei.com>
>
> ---
>
> 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140512/7dc44335/attachment-0001.htm>