On 05/13/2014 01:51 PM, Laine Stump wrote:
> Inspired by a simpler patch from "Wangrui (K) <moon.wangrui@xxxxxxxxxx>".
>
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list