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

[John Ferlan]

On 08/02/2018 10:07 AM, Matthias Bolte wrote:
> 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.
> 

That patch was confusing at best... The following works just fine:

diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index cee98ebcaf..a3982089e3 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -3387,6 +3387,8 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
         esxVI_Int_Alloc(&newPowerInfo->startOrder) < 0 ||
         esxVI_Int_Alloc(&newPowerInfo->startDelay) < 0 ||
         esxVI_Int_Alloc(&newPowerInfo->stopDelay) < 0) {
+
+        esxVI_AutoStartPowerInfo_Free(&newPowerInfo);
         goto cleanup;
     }
 
@@ -3403,8 +3405,6 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
         goto cleanup;
     }
 
-    newPowerInfo = NULL;
-
     if (esxVI_ReconfigureAutostart
           (priv->primary,
            priv->primary->hostSystem->configManager->autoStartManager,
@@ -3426,8 +3426,6 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
     esxVI_AutoStartDefaults_Free(&defaults);
     esxVI_AutoStartPowerInfo_Free(&powerInfoList);
 
-    esxVI_AutoStartPowerInfo_Free(&newPowerInfo);
-
     return result;
 }


A comment could be added that indicates by moving the *_Free to cleanup:
along with the setting of newPowerInfo = NULL after the AppendToList
caused issues with eventual esxVI_HostAutoStartManagerConfig_Free trying
to VIR_FREE static strings and double freeing the machine object.

John