On Thu, Aug 02, 2018 at 05:37:46PM +0200, Matthias Bolte wrote: > 2018-08-02 16:45 GMT+02:00 John Ferlan <jferlan@xxxxxxxxxx>: > > > > > > On 08/02/2018 10:07 AM, Matthias Bolte wrote: > >> 2018-08-02 15:20 GMT+02:00 John Ferlan <jferlan@xxxxxxxxxx>: > >>> > >>> > >>> 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@xxxxxxxxx>: > >>>>> 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@xxxxxxxxxxxxxx> > 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@xxxxxxxxxxxxxx> > --- > 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@xxxxxxxxx> -- Thanks, Marcos -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
