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

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



On Thu, Aug 02, 2018 at 09:23:05PM +0200, Matthias Bolte wrote:
> 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

Thanks Mathias. I created a bug to backport this fix to Fedora 28:
https://bugzilla.redhat.com/show_bug.cgi?id=1611921

-- 
Thanks,
	Marcos


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