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

2018-08-02 16:45 GMT+02:00 John Ferlan <jferlan(a)redhat.com&gt;: > > > On 08/02/2018 10:07 AM, Matthias Bolte wrote: >> 2018-08-02 15:20 GMT+02:00 John Ferlan <jferlan(a)redhat.com&gt;: >>> >>> >>> 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(a)gmail.com&gt;: >>>>> 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(a)googlemail.com&gt; 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(a)googlemail.com&gt; --- 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