[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] esx: avoid null dereference on error



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/

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]