[libvirt] [PATCH 0/2] fix two issues found by clang
Pavel Hrdina (2): xenapi_driver: fix copy-paste typo esx_vi: fix possible segfault src/esx/esx_vi.c | 3 ++- src/xenapi/xenapi_driver.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) -- 2.0.5
Clang found that we are passing variable with wrong enum type to 'xenapiCrashExitEnum2virDomainLifecycle' function. This is probably copy-paste typo as the correct variable exists in the code, but it isn't used. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/xenapi/xenapi_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index c5118ec..cef53ec 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1504,7 +1504,7 @@ xenapiDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) defPtr->onReboot = xenapiNormalExitEnum2virDomainLifecycle(action); enum xen_on_crash_behaviour crash; if (xen_vm_get_actions_after_crash(session, &crash, vm)) - defPtr->onCrash = xenapiCrashExitEnum2virDomainLifecycle(action); + defPtr->onCrash = xenapiCrashExitEnum2virDomainLifecycle(crash); xen_vm_get_platform(session, &result, vm); if (result != NULL) { size_t i; -- 2.0.5
On Wed, Jan 21, 2015 at 18:09:27 +0100, Pavel Hrdina wrote:
Clang found that we are passing variable with wrong enum type to 'xenapiCrashExitEnum2virDomainLifecycle' function. This is probably copy-paste typo as the correct variable exists in the code, but it isn't used.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/xenapi/xenapi_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK, safe for release. Peter
On 01/21/2015 06:29 PM, Peter Krempa wrote:
On Wed, Jan 21, 2015 at 18:09:27 +0100, Pavel Hrdina wrote:
Clang found that we are passing variable with wrong enum type to 'xenapiCrashExitEnum2virDomainLifecycle' function. This is probably copy-paste typo as the correct variable exists in the code, but it isn't used.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/xenapi/xenapi_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK, safe for release.
Peter
Pushed, thanks Pavel
Clang found possible dereference of NULL pointer which is right. Function 'esxVI_LookupTaskInfoByTask' should find a task info. The issue is that we could return 0 and leave 'taksInfo' pointer NULL because if there is no match we simply end the search loop end set 'result' to 0. Every caller count on the fact that if the return value is 0 than it's safe to dereference 'taskInfo'. We should return 0 only in case we found something and the '*taskInfo' is not NULL. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/esx/esx_vi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index a87f2c0..752d32a 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -3298,7 +3298,8 @@ esxVI_LookupTaskInfoByTask(esxVI_Context *ctx, } } - result = 0; + if (*taskInfo) + result = 0; cleanup: esxVI_String_Free(&propertyNameList); -- 2.0.5
On Wed, Jan 21, 2015 at 18:09:28 +0100, Pavel Hrdina wrote:
Clang found possible dereference of NULL pointer which is right. Function 'esxVI_LookupTaskInfoByTask' should find a task info. The issue is that we could return 0 and leave 'taksInfo' pointer NULL because if there is no match we simply end the search loop end set 'result' to 0. Every caller count on the fact that if the return value is 0 than it's safe to dereference 'taskInfo'. We should return 0 only in case we found something and the '*taskInfo' is not NULL.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/esx/esx_vi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index a87f2c0..752d32a 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -3298,7 +3298,8 @@ esxVI_LookupTaskInfoByTask(esxVI_Context *ctx, } }
- result = 0; + if (*taskInfo) + result = 0;
I think a more obvious fix would be to move setting of result to zero into the condition that breaks the loop. In that case, result gets set to 0 only if the entry was found.
cleanup: esxVI_String_Free(&propertyNameList);
ACK with or without that change. I think that this wasn't discovered due to the fact that the correct entry is always in the list and is always first as we haven't seen reports for spamming the warning. Peter
On 01/21/2015 06:47 PM, Peter Krempa wrote:
On Wed, Jan 21, 2015 at 18:09:28 +0100, Pavel Hrdina wrote:
Clang found possible dereference of NULL pointer which is right. Function 'esxVI_LookupTaskInfoByTask' should find a task info. The issue is that we could return 0 and leave 'taksInfo' pointer NULL because if there is no match we simply end the search loop end set 'result' to 0. Every caller count on the fact that if the return value is 0 than it's safe to dereference 'taskInfo'. We should return 0 only in case we found something and the '*taskInfo' is not NULL.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/esx/esx_vi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index a87f2c0..752d32a 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -3298,7 +3298,8 @@ esxVI_LookupTaskInfoByTask(esxVI_Context *ctx, } }
- result = 0; + if (*taskInfo) + result = 0;
I think a more obvious fix would be to move setting of result to zero into the condition that breaks the loop. In that case, result gets set to 0 only if the entry was found.
cleanup: esxVI_String_Free(&propertyNameList);
ACK with or without that change. I think that this wasn't discovered due to the fact that the correct entry is always in the list and is always first as we haven't seen reports for spamming the warning.
Peter
Pushed with the update that you've proposed, thanks. Pavel
participants (2)
-
Pavel Hrdina -
Peter Krempa