[libvirt] [PATCH] util: refactor virNetlinkCommand to fix several bugs / style problems

[Ján Tomko]

On 05/13/2014 01:51 PM, Laine Stump wrote:
> Inspired by a simpler patch from "Wangrui (K) <moon.wangrui at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140516/0bd0c2a4/attachment-0001.sig>