Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
2018-08-02 18:16 GMT+02:00 John Ferlan :
>
>
> 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 :
On 08/02/2018 10:07 AM, Matthias Bolte wrote:
> 2018-08-02 15:20 GMT+02:00 John Ferlan :
>>
>>
>> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
>>> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
>>> :
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/msg0.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(>powerInfo,
>> newPowerInfo) < 0) {
>> goto cleanup;
>> }
>>
>> newPowerInfo = NULL;
>>
>> and following it to esxVI_List_Append which on success would seemingly
>> assign @newPowerInfo into the >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() < 0 || " condition
>> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
>> to cleanup, removing it from the cleanup: path, and then remove the
>> "newPowerInfo = NULL;" after list insertion.
>>
>>
>>
>> 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(>startOrder) < 0 ||
esxVI_Int_Alloc(>startDelay) < 0 ||
esxVI_Int_Alloc(>stopDelay) < 0) {
+
+esxVI_AutoStartPowerInfo_Free();
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();
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
On Thu, Aug 02, 2018 at 09:23:05PM +0200, Matthias Bolte wrote:
> 2018-08-02 18:16 GMT+02:00 John Ferlan :
> >
> >
> > 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 :
>
>
> On 08/02/2018 10:07 AM, Matthias Bolte wrote:
> > 2018-08-02 15:20 GMT+02:00 John Ferlan :
> >>
> >>
> >> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
> >>> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
> >>> :
> 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/msg0.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(>powerInfo,
> >> newPowerInfo) < 0) {
> >> goto cleanup;
> >> }
> >>
> >> newPowerInfo = NULL;
> >>
> >> and following it to esxVI_List_Append which on success would seemingly
> >> assign @newPowerInfo into the >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() < 0 || " condition
> >> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
> >> to cleanup, removing it from the cleanup: path, and then remove the
> >> "newPowerInfo = NULL;" after list insertion.
> >>
> >>
> >>
> >> 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(>startOrder) < 0 ||
> esxVI_Int_Alloc(>startDelay) < 0 ||
> esxVI_Int_Alloc(>stopDelay) < 0) {
> +
> +esxVI_AutoStartPowerInfo_Free();
> goto cleanup;
> }
>
> @@ -3403,8 +3405,6 @@ esxDomainSetAutostart(virDomainPtr domain, int
> autostart)
> goto cleanup;
> }
>
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
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 :
>>>
>>>
>>> On 08/02/2018 10:07 AM, Matthias Bolte wrote:
2018-08-02 15:20 GMT+02:00 John Ferlan :
>
>
> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
>> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
>> :
>>> 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/msg0.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(>powerInfo,
> newPowerInfo) < 0) {
> goto cleanup;
> }
>
> newPowerInfo = NULL;
>
> and following it to esxVI_List_Append which on success would seemingly
> assign @newPowerInfo into the >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() < 0 || " condition
> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
> to cleanup, removing it from the cleanup: path, and then remove the
> "newPowerInfo = NULL;" after list insertion.
>
>
>
> 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(>startOrder) < 0 ||
>>> esxVI_Int_Alloc(>startDelay) < 0 ||
>>> esxVI_Int_Alloc(>stopDelay) < 0) {
>>> +
>>> +esxVI_AutoStartPowerInfo_Free();
>>> 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();
>>> esxVI_AutoStartPowerInfo_Free();
>>>
>>> -esxVI_AutoStartPowerInfo_Free();
>>> -
>>> return result;
>>> }
>>>
>>>
>>> A comment could be added that
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
On Thu, Aug 02, 2018 at 05:37:46PM +0200, Matthias Bolte wrote:
> 2018-08-02 16:45 GMT+02:00 John Ferlan :
> >
> >
> > On 08/02/2018 10:07 AM, Matthias Bolte wrote:
> >> 2018-08-02 15:20 GMT+02:00 John Ferlan :
> >>>
> >>>
> >>> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
> :
> > 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/msg0.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(>powerInfo,
> >>> newPowerInfo) < 0) {
> >>> goto cleanup;
> >>> }
> >>>
> >>> newPowerInfo = NULL;
> >>>
> >>> and following it to esxVI_List_Append which on success would seemingly
> >>> assign @newPowerInfo into the >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() < 0 || " condition
> >>> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
> >>> to cleanup, removing it from the cleanup: path, and then remove the
> >>> "newPowerInfo = NULL;" after list insertion.
> >>>
> >>>
> >>>
> >>> 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(>startOrder) < 0 ||
> > esxVI_Int_Alloc(>startDelay) < 0 ||
> > esxVI_Int_Alloc(>stopDelay) < 0) {
> > +
> > +esxVI_AutoStartPowerInfo_Free();
> > 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();
> > esxVI_AutoStartPowerInfo_Free();
> >
> > -esxVI_AutoStartPowerInfo_Free();
> > -
> > return result;
> > }
> >
> >
> > A comment could be added that indicates by moving the *_Free to cleanup:
> > along with
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
2018-08-02 16:45 GMT+02:00 John Ferlan :
>
>
> On 08/02/2018 10:07 AM, Matthias Bolte wrote:
>> 2018-08-02 15:20 GMT+02:00 John Ferlan :
>>>
>>>
>>> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
:
> 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/msg0.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(>powerInfo,
>>> newPowerInfo) < 0) {
>>> goto cleanup;
>>> }
>>>
>>> newPowerInfo = NULL;
>>>
>>> and following it to esxVI_List_Append which on success would seemingly
>>> assign @newPowerInfo into the >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() < 0 || " condition
>>> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
>>> to cleanup, removing it from the cleanup: path, and then remove the
>>> "newPowerInfo = NULL;" after list insertion.
>>>
>>>
>>>
>>> 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(>startOrder) < 0 ||
> esxVI_Int_Alloc(>startDelay) < 0 ||
> esxVI_Int_Alloc(>stopDelay) < 0) {
> +
> +esxVI_AutoStartPowerInfo_Free();
> 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();
> esxVI_AutoStartPowerInfo_Free();
>
> -esxVI_AutoStartPowerInfo_Free();
> -
> 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
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 :
>>
>>
>> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
>>> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
>>> :
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/msg0.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(>powerInfo,
>> newPowerInfo) < 0) {
>> goto cleanup;
>> }
>>
>> newPowerInfo = NULL;
>>
>> and following it to esxVI_List_Append which on success would seemingly
>> assign @newPowerInfo into the >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() < 0 || " condition
>> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
>> to cleanup, removing it from the cleanup: path, and then remove the
>> "newPowerInfo = NULL;" after list insertion.
>>
>>
>>
>> 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(>startOrder) < 0 ||
esxVI_Int_Alloc(>startDelay) < 0 ||
esxVI_Int_Alloc(>stopDelay) < 0) {
+
+esxVI_AutoStartPowerInfo_Free();
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();
esxVI_AutoStartPowerInfo_Free();
-esxVI_AutoStartPowerInfo_Free();
-
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
2018-08-02 15:20 GMT+02:00 John Ferlan :
>
>
> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
>> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
>> :
>>> 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/msg0.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(>powerInfo,
> newPowerInfo) < 0) {
> goto cleanup;
> }
>
> newPowerInfo = NULL;
>
> and following it to esxVI_List_Append which on success would seemingly
> assign @newPowerInfo into the >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() < 0 || " condition
> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
> to cleanup, removing it from the cleanup: path, and then remove the
> "newPowerInfo = NULL;" after list insertion.
>
>
>
> 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
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
---
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
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 :
>> 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/msg0.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(>powerInfo,
newPowerInfo) < 0) {
goto cleanup;
}
newPowerInfo = NULL;
and following it to esxVI_List_Append which on success would seemingly
assign @newPowerInfo into the >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() < 0 || " condition
cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
to cleanup, removing it from the cleanup: path, and then remove the
"newPowerInfo = NULL;" after list insertion.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza :
> 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/msg0.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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[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/msg0.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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
