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

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