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

[Laine Stump]

On 05/16/2014 06:24 PM, Ján Tomko wrote:
> 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?

Good point - yet another existing bug that can be fixed in this patch.
I'll add an "if (len == 0)" clause that prints out a plain error (since
for us receiving nothing *is* an error).

>
>>          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]

Sure, I can do that.

I pushed the patch with those two changes. Thanks for the attentive review!