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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
