[libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart

[Matthias Bolte]

2018-08-02 15:20 GMT+02:00 John Ferlan <jferlan at redhat.com>:
>
>
> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
>> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza <marcos.souza.org at gmail.com>:
>>> This is a new version from the last patchset sent yesterday, but now using
>>> VIR_STRNDUP, instead of allocating memory manually.
>>>
>>> First version: https://www.redhat.com/archives/libvir-list/2018-August/msg00000.html
>>>
>>> Marcos Paulo de Souza (2):
>>>   esx: Do not crash SetAutoStart by double free
>>>   esx: Fix SetAutoStart invalid pointer free
>>>
>>>  src/esx/esx_driver.c | 14 +++++++++++---
>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> I see the problem, but your approach is too complicated.
>>
>> There is already code in place to handle those situations:
>>
>> 3417  cleanup:
>> 3418     if (newPowerInfo) {
>> 3419         newPowerInfo->key = NULL;
>> 3420         newPowerInfo->startAction = NULL;
>> 3421         newPowerInfo->stopAction = NULL;
>> 3422     }
>>
>> That resets those fields to NULL to avoid double freeing and freeing
>> static strings.
>>
>> The problem is that commit ae83e02f3dd7fe99fed5d8159a35b666fafeafd5 by
>> John Frelan broke this logic, by setting newPowerInfo to NULL in the
>> success path, to silence Coverity.
>>
>
> Memory recall from "Tue Sep 3 07:20:34 2013 -0400" is perhaps impossible
> ;-)...  TL;DR, looks like the error back then was a false positive
> because (as I know I've learned since then) Coverity has a hard time
> when a boolean or size_t count is used to control whether or not memory
> would be freed.
>
> Looking at the logic:
>
>     if (esxVI_AutoStartPowerInfo_AppendToList(&spec->powerInfo,
>                                               newPowerInfo) < 0) {
>         goto cleanup;
>     }
>
>     newPowerInfo = NULL;
>
> and following it to esxVI_List_Append which on success would seemingly
> assign @newPowerInfo into the &spec->powerInfo list, I can certainly see
> why logically setting newPowerInfo = NULL after than would seem to be
> the right thing. Of course, I see now the subtle reason why it's not a
> good idea.
>
> Restoring logic from that commit in esxDomainSetAutostart, then Coverity
> believes that @newPowerInfo is leaked from the goto cleanup at
> allocation because for some reason it has decided it must evaluate both
> true and false of a condition even though the logic only ever set the
> boolean when @newPowerInfo was placed onto the @spec->powerInfo list.
> IOW: A false positive because the human can read the code and say that
> Coverity is full of it.
>
> So either I add this to my Coverity list of false positives or in the
> "if (esxVI_AutoStartPowerInfo_Alloc(&newPowerInfo) < 0 || " condition
> cleanup logic call esxVI_AutoStartPowerInfo_Free(&newPowerInfo); prior
> to cleanup, removing it from the cleanup: path, and then remove the
> "newPowerInfo = NULL;" after list insertion.
>
> <sigh>
>
> John

Okay, I see what's going on. I suggest this alternative patch that
keeps the newPowerInfo = NULL line to make Coverity understand the
cleanup code, but adds a second variable to restore the original
logic. I hope this doesn't upset Coverity.

Marcos, can you give the attached patch a try? It should fix the
problems you try to fix here, by restoring the original cleanup logic.

-- 
Matthias Bolte
http://photron.blogspot.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-esx-Fix-double-free-and-freeing-static-strings-in-es.patch
Type: text/x-patch
Size: 3257 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180802/ca97b623/attachment-0001.bin>