Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
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>:
> 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