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

[Matthias Bolte]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-esx-Fix-double-free-and-freeing-static-strings-in-es-v2.patch
Type: text/x-patch
Size: 2604 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180802/855b518a/attachment-0001.bin>