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

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