[libvirt] [PATCH] esx: avoid null dereference on error
Detected by clang. * src/esx/esx_driver.c (esxDomainGetInfo): Fail early on error. --- src/esx/esx_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1f8f90b..e929208 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -2372,8 +2372,9 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) if (perfEntityMetric == NULL) { VIR_ERROR(_("QueryPerf returned object with unexpected type '%s'"), esxVI_Type_ToString(perfEntityMetricBase->_type)); + goto cleanup; } perfMetricIntSeries = esxVI_PerfMetricIntSeries_DynamicCast(perfEntityMetric->value); -- 1.7.4.4
On 05/03/2011 03:10 PM, Eric Blake wrote:
Detected by clang.
* src/esx/esx_driver.c (esxDomainGetInfo): Fail early on error. --- src/esx/esx_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1f8f90b..e929208 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -2372,8 +2372,9 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
if (perfEntityMetric == NULL) { VIR_ERROR(_("QueryPerf returned object with unexpected type '%s'"), esxVI_Type_ToString(perfEntityMetricBase->_type)); + goto cleanup; }
perfMetricIntSeries = esxVI_PerfMetricIntSeries_DynamicCast(perfEntityMetric->value);
I would just say ACK, since this obviously eliminates a null dereference, but I notice that the following check for perfMetricIntSeries == NULL also calls VIR_ERROR and then doesn't goto cleanup, so I'm wondering if maybe the intent is that if either of these is NULL, result should still get set to 0. Mathias?
2011/5/4 Laine Stump <laine@laine.org>:
On 05/03/2011 03:10 PM, Eric Blake wrote:
Detected by clang.
* src/esx/esx_driver.c (esxDomainGetInfo): Fail early on error. --- src/esx/esx_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1f8f90b..e929208 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -2372,8 +2372,9 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
if (perfEntityMetric == NULL) { VIR_ERROR(_("QueryPerf returned object with unexpected type '%s'"),
esxVI_Type_ToString(perfEntityMetricBase->_type)); + goto cleanup; }
perfMetricIntSeries =
esxVI_PerfMetricIntSeries_DynamicCast(perfEntityMetric->value);
I would just say ACK, since this obviously eliminates a null dereference, but I notice that the following check for perfMetricIntSeries == NULL also calls VIR_ERROR and then doesn't goto cleanup, so I'm wondering if maybe the intent is that if either of these is NULL, result should still get set to 0. Mathias?
NACK, as written. There is a potential NULL dereference in there, but just going to cleanup results in freeing static strings here. Patch 1 fixes it correctly. Actually this code has been there for a while now, but didn't do anything useful with the queried values because of the format mismatch between libvirt and ESX. Therefore, patch 2 disables that code but keeps it as a reference for how to query performance counters. Matthias
On Wed, May 04, 2011 at 08:38:43AM +0200, Matthias Bolte wrote:
2011/5/4 Laine Stump <laine@laine.org>:
On 05/03/2011 03:10 PM, Eric Blake wrote:
Detected by clang.
* src/esx/esx_driver.c (esxDomainGetInfo): Fail early on error. --- src/esx/esx_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1f8f90b..e929208 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -2372,8 +2372,9 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
if (perfEntityMetric == NULL) { VIR_ERROR(_("QueryPerf returned object with unexpected type '%s'"),
esxVI_Type_ToString(perfEntityMetricBase->_type)); + goto cleanup; }
perfMetricIntSeries =
esxVI_PerfMetricIntSeries_DynamicCast(perfEntityMetric->value);
I would just say ACK, since this obviously eliminates a null dereference, but I notice that the following check for perfMetricIntSeries == NULL also calls VIR_ERROR and then doesn't goto cleanup, so I'm wondering if maybe the intent is that if either of these is NULL, result should still get set to 0. Mathias?
NACK, as written.
There is a potential NULL dereference in there, but just going to cleanup results in freeing static strings here. Patch 1 fixes it correctly.
Actually this code has been there for a while now, but didn't do anything useful with the queried values because of the format mismatch between libvirt and ESX. Therefore, patch 2 disables that code but keeps it as a reference for how to query performance counters.
If code is not used, then I would ACK disabling for 0.9.1 and then we can figure out if this really should eb provided in some other ways. My only worry is a possible regression, you decide :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2011/5/4 Daniel Veillard <veillard@redhat.com>:
On Wed, May 04, 2011 at 08:38:43AM +0200, Matthias Bolte wrote:
2011/5/4 Laine Stump <laine@laine.org>:
On 05/03/2011 03:10 PM, Eric Blake wrote:
Detected by clang.
* src/esx/esx_driver.c (esxDomainGetInfo): Fail early on error. --- src/esx/esx_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1f8f90b..e929208 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -2372,8 +2372,9 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
if (perfEntityMetric == NULL) { VIR_ERROR(_("QueryPerf returned object with unexpected type '%s'"),
esxVI_Type_ToString(perfEntityMetricBase->_type)); + goto cleanup; }
perfMetricIntSeries =
esxVI_PerfMetricIntSeries_DynamicCast(perfEntityMetric->value);
I would just say ACK, since this obviously eliminates a null dereference, but I notice that the following check for perfMetricIntSeries == NULL also calls VIR_ERROR and then doesn't goto cleanup, so I'm wondering if maybe the intent is that if either of these is NULL, result should still get set to 0. Mathias?
NACK, as written.
There is a potential NULL dereference in there, but just going to cleanup results in freeing static strings here. Patch 1 fixes it correctly.
Actually this code has been there for a while now, but didn't do anything useful with the queried values because of the format mismatch between libvirt and ESX. Therefore, patch 2 disables that code but keeps it as a reference for how to query performance counters.
If code is not used, then I would ACK disabling for 0.9.1 and then we can figure out if this really should eb provided in some other ways. My only worry is a possible regression, you decide :-)
Daniel
Okay, I've tested this two patches now that I have access to an ESX server again and everything still works as expected. So I decide to push them for 0.9.1 :) Matthias
participants (4)
-
Daniel Veillard -
Eric Blake -
Laine Stump -
Matthias Bolte