On Thu, Aug 02, 2018 at 09:23:05PM +0200, Matthias Bolte wrote: > 2018-08-02 18:16 GMT+02:00 John Ferlan <jferlan@xxxxxxxxxx>: > > > > > > 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@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> > >> > > > > Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx> > > > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
