[libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
Marcos Paulo de Souza
marcos.souza.org at gmail.com
Thu Aug 2 16:11:14 UTC 2018
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>
--
Thanks,
Marcos
More information about the libvir-list
mailing list