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

[John Ferlan]

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