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

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.