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.
--
Matthias Bolte
http://photron.blogspot.com
From 9d7262174142ec0cbe47ead39896a82e9155a464 Mon Sep 17 00:00:00 2001
From: Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>
Date: Thu, 2 Aug 2018 15:57:32 +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.
Restore the original logic by adding a second variable.
Signed-off-by: Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>
---
src/esx/esx_driver.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index cee98ebcaf..a07a28ac93 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -3332,6 +3332,7 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
esxVI_AutoStartPowerInfo *powerInfoList = NULL;
esxVI_AutoStartPowerInfo *powerInfo = NULL;
esxVI_AutoStartPowerInfo *newPowerInfo = NULL;
+ esxVI_AutoStartPowerInfo *newPowerInfo_helper = NULL;
if (esxVI_EnsureSession(priv->primary) < 0)
return -1;
@@ -3398,6 +3399,13 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
newPowerInfo->stopDelay->value = -1; /* use system default */
newPowerInfo->stopAction = (char *)"none";
+ /* Assign the new AutoStartPowerInfo to a second helper variable, so that
+ * the original variable can be used to keep track of ownership and whether
+ * or not the AutoStartPowerInfo object has to be freed explicitly in the
+ * cleanup section. The helper variable is used to unset some fields of the
+ * AutoStartPowerInfo object before it is freed, independent of ownership. */
+ newPowerInfo_helper = newPowerInfo;
+
if (esxVI_AutoStartPowerInfo_AppendToList(&spec->powerInfo,
newPowerInfo) < 0) {
goto cleanup;
@@ -3415,17 +3423,22 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
result = 0;
cleanup:
- if (newPowerInfo) {
- newPowerInfo->key = NULL;
- newPowerInfo->startAction = NULL;
- newPowerInfo->stopAction = NULL;
+ if (newPowerInfo_helper) {
+ /* Unset the key to stop the esxVI_HostAutoStartManagerConfig_Free call
+ * internally from double-freeing the virtualMachine object already
+ * freed by the esxVI_ObjectContent_Free call before. Also unset the
+ * startAction and stopAction as those point to static strings that the
+ * esxVI_HostAutoStartManagerConfig_Free call would internally try to
+ * free as well. */
+ newPowerInfo_helper->key = NULL;
+ newPowerInfo_helper->startAction = NULL;
+ newPowerInfo_helper->stopAction = NULL;
}
esxVI_ObjectContent_Free(&virtualMachine);
esxVI_HostAutoStartManagerConfig_Free(&spec);
esxVI_AutoStartDefaults_Free(&defaults);
esxVI_AutoStartPowerInfo_Free(&powerInfoList);
-
esxVI_AutoStartPowerInfo_Free(&newPowerInfo);
return result;
--
2.14.1
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
