[libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
John Ferlan
jferlan at redhat.com
Thu Aug 2 16:16:23 UTC 2018
On 08/02/2018 12:11 PM, Marcos Paulo de Souza wrote:
> On Thu, Aug 02, 2018 at 05:37:46PM +0200, Matthias Bolte wrote:
>> 2018-08-02 16:45 GMT+02:00 John Ferlan <jferlan at redhat.com>:
>>>
>>>
>>> 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
>>
>> Your approach works, but it doesn't handle the
>> esxVI_AutoStartPowerInfo_AppendToList cleanup case in which
>> key/startAction/stopAction have to be unset and
>> esxVI_AutoStartPowerInfo_Free(&newPowerInfo) has to be called because
>> the list failed to take ownership of the newPowerInfo object and
>> esxVI_HostAutoStartManagerConfig_Free will not free it in this case.
>>
>> Based on your suggestion here's a modified patch for this.
>>
>> --
>> Matthias Bolte
>> http://photron.blogspot.com
>
>> From 090cf89ca6d8966fccf6cf6110ac8dc0b79865a8 Mon Sep 17 00:00:00 2001
>> From: Matthias Bolte <matthias.bolte at googlemail.com>
>> Date: Thu, 2 Aug 2018 17:33:37 +0200
>> Subject: [PATCH] esx: Fix double-free and freeing static strings in
>> esxDomainSetAutostart
>>
>> Since commit ae83e02f3dd7fe99fed5d8159a35b666fafeafd5#l3393 the
>> newPowerInfo pointer itself is used to track the ownership of the
>> AutoStartPowerInfo object to make Coverity understand the code better.
>> This broke the code that unset some members of the AutoStartPowerInfo
>> object that should not be freed the normal way.
>>
>> Instead, transfer ownership of the AutoStartPowerInfo object to the
>> HostAutoStartManagerConfig object before filling in the values that
>> need special handling. This allows to free the AutoStartPowerInfo
>> directly without having to deal with the special values, or to let
>> the old (now restored) logic handle the special values again.
>>
>> Signed-off-by: Matthias Bolte <matthias.bolte at googlemail.com>
>> ---
>> src/esx/esx_driver.c | 14 ++++----------
>> 1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
>> index cee98ebcaf..c2154799fa 100644
>> --- a/src/esx/esx_driver.c
>> +++ b/src/esx/esx_driver.c
>> @@ -3386,7 +3386,10 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
>> if (esxVI_AutoStartPowerInfo_Alloc(&newPowerInfo) < 0 ||
>> esxVI_Int_Alloc(&newPowerInfo->startOrder) < 0 ||
>> esxVI_Int_Alloc(&newPowerInfo->startDelay) < 0 ||
>> - esxVI_Int_Alloc(&newPowerInfo->stopDelay) < 0) {
>> + esxVI_Int_Alloc(&newPowerInfo->stopDelay) < 0 ||
>> + esxVI_AutoStartPowerInfo_AppendToList(&spec->powerInfo,
>> + newPowerInfo) < 0) {
>> + esxVI_AutoStartPowerInfo_Free(&newPowerInfo);
>> goto cleanup;
>> }
>>
>> @@ -3398,13 +3401,6 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
>> newPowerInfo->stopDelay->value = -1; /* use system default */
>> newPowerInfo->stopAction = (char *)"none";
>>
>> - if (esxVI_AutoStartPowerInfo_AppendToList(&spec->powerInfo,
>> - newPowerInfo) < 0) {
>> - goto cleanup;
>> - }
>> -
>> - newPowerInfo = NULL;
>> -
>> if (esxVI_ReconfigureAutostart
>> (priv->primary,
>> priv->primary->hostSystem->configManager->autoStartManager,
>> @@ -3426,8 +3422,6 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
>> esxVI_AutoStartDefaults_Free(&defaults);
>> esxVI_AutoStartPowerInfo_Free(&powerInfoList);
>>
>> - esxVI_AutoStartPowerInfo_Free(&newPowerInfo);
>> -
>> return result;
>> }
>>
>> --
>> 2.14.1
>>
>
> It worked in ESXi server. So:
>
> Tested-by: Marcos Paulo de Souza <marcos.souza.org at gmail.com>
>
Reviewed-by: John Ferlan <jferlan at redhat.com>
and safe for freeze (you have commit rights, so I'll let you push).
Also checked w/ my coverity build and no issue from there.
John
More information about the libvir-list
mailing list