[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



2018-08-02 18:16 GMT+02:00 John Ferlan <jferlan redhat com>:
>
>
> 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 redhat com>:
>>>>
>>>>
>>>> On 08/02/2018 10:07 AM, Matthias Bolte wrote:
>>>>> 2018-08-02 15:20 GMT+02:00 John Ferlan <jferlan 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 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 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 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 gmail com>
>>
>
> Reviewed-by: John Ferlan <jferlan 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
>

Thanks and pushed.

-- 
Matthias Bolte
http://photron.blogspot.com


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]