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@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?
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!
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list