[libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
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(-) -- 2.17.1
SetAutoStart method cannot free virtualMachine using esxVI_ObjectContent_Free, since: esxVI_HostAutoStartManagerConfig_Free -> esxVI_AutoStartPowerInfo_Free -> esxVI_ManagedObjectReference_Free(item->key); item->key, in this context, is virtualMachine->obj, so calling esxVI_ObjectContent_Free creates a double free, becasuse esxVI_ObjectContent_Free also calls esxVI_ManagedObjectReference_Free(&item->obj). Removing the esxVI_ObjectContent_Free from SetAutoStart fixes this problem. Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index cee98ebcaf..3835e4cb3c 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3421,7 +3421,9 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart) newPowerInfo->stopAction = NULL; } - esxVI_ObjectContent_Free(&virtualMachine); + /* HostAutoStartManagerConfig free method will call autoStartPowerInfoFree + * in order to free virtualMachine, since newPowerInfo-> key points to + * virtualMachine */ esxVI_HostAutoStartManagerConfig_Free(&spec); esxVI_AutoStartDefaults_Free(&defaults); esxVI_AutoStartPowerInfo_Free(&powerInfoList); -- 2.17.1
esxVI_AutoStartPowerInfo_Free, which is called from esxVI_HostAutoStartManagerConfig_Free, will always call VIR_FREE to free memory from {start,stop}Action, leading to a invalid pointer. With this patch applied, ESX can set autostart successfully to all it's domains. Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- Changes from v1: * Stop calling VIR_ALLOC_N and strcpy, and use VIR_STRNDUP instead src/esx/esx_driver.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 3835e4cb3c..dc07cf8770 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3394,9 +3394,15 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart) newPowerInfo->startOrder->value = -1; /* no specific start order */ newPowerInfo->startDelay->value = -1; /* use system default */ newPowerInfo->waitForHeartbeat = esxVI_AutoStartWaitHeartbeatSetting_SystemDefault; - newPowerInfo->startAction = autostart ? (char *)"powerOn" : (char *)"none"; newPowerInfo->stopDelay->value = -1; /* use system default */ - newPowerInfo->stopAction = (char *)"none"; + + /* startAction and stopAction will be freed by esxVI_HostAutoStartManagerConfig_Free */ + if (VIR_STRNDUP(newPowerInfo->startAction, autostart ? "powerOn" : "none", + autostart ? 7 : 4) < 1) + goto cleanup; + + if (VIR_STRNDUP(newPowerInfo->stopAction, "none", 4) < 1) + goto cleanup; if (esxVI_AutoStartPowerInfo_AppendToList(&spec->powerInfo, newPowerInfo) < 0) { -- 2.17.1
2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza <marcos.souza.org@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. -- Matthias Bolte http://photron.blogspot.com
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@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
2018-08-02 15:20 GMT+02:00 John Ferlan <jferlan@redhat.com>:
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@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
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
On 08/02/2018 10:07 AM, Matthias Bolte wrote:
2018-08-02 15:20 GMT+02:00 John Ferlan <jferlan@redhat.com>:
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@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
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
2018-08-02 16:45 GMT+02:00 John Ferlan <jferlan@redhat.com>:
On 08/02/2018 10:07 AM, Matthias Bolte wrote:
2018-08-02 15:20 GMT+02:00 John Ferlan <jferlan@redhat.com>:
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@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
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
On Thu, Aug 02, 2018 at 05:37:46PM +0200, Matthias Bolte wrote:
2018-08-02 16:45 GMT+02:00 John Ferlan <jferlan@redhat.com>:
On 08/02/2018 10:07 AM, Matthias Bolte wrote:
2018-08-02 15:20 GMT+02:00 John Ferlan <jferlan@redhat.com>:
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@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
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@googlemail.com> 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@googlemail.com> --- 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@gmail.com> -- Thanks, Marcos
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@redhat.com>:
On 08/02/2018 10:07 AM, Matthias Bolte wrote:
2018-08-02 15:20 GMT+02:00 John Ferlan <jferlan@redhat.com>:
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@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
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@googlemail.com> 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@googlemail.com> --- 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@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com> 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
2018-08-02 18:16 GMT+02:00 John Ferlan <jferlan@redhat.com>:
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@redhat.com>:
On 08/02/2018 10:07 AM, Matthias Bolte wrote:
2018-08-02 15:20 GMT+02:00 John Ferlan <jferlan@redhat.com>:
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@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
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@googlemail.com> 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@googlemail.com> --- 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@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
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
Thanks and pushed. -- Matthias Bolte http://photron.blogspot.com
On Thu, Aug 02, 2018 at 09:23:05PM +0200, Matthias Bolte wrote:
2018-08-02 18:16 GMT+02:00 John Ferlan <jferlan@redhat.com>:
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@redhat.com>:
On 08/02/2018 10:07 AM, Matthias Bolte wrote:
2018-08-02 15:20 GMT+02:00 John Ferlan <jferlan@redhat.com>: > > > 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@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
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@googlemail.com> 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@googlemail.com> --- 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@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
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
Thanks and pushed.
-- Matthias Bolte http://photron.blogspot.com
Thanks Mathias. I created a bug to backport this fix to Fedora 28: https://bugzilla.redhat.com/show_bug.cgi?id=1611921 -- Thanks, Marcos
participants (3)
-
John Ferlan -
Marcos Paulo de Souza -
Matthias Bolte