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

[Matthias Bolte <matthias bolte googlemail com>]

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