Re: [libvirt] [PATCH] esx_vmx.c: don't dereference a NULL virtualDev
2009/12/15 Daniel P. Berrange <berrange(a)redhat.com>:
On Tue, Dec 15, 2009 at 04:43:11PM +0100, Matthias Bolte wrote:
> 2009/12/15 Jim Meyering <jim(a)meyering.net>:
> > The offending code starts here:
> >
> > int
> > esxVMX_ParseSCSIController(virConnectPtr conn, virConfPtr conf, int controller,
> > int *present, char **virtualDev)
> > {
> > char present_name[32];
> > char virtualDev_name[32];
> >
> > if (virtualDev == NULL || *virtualDev != NULL) {
> > ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Invalid argument");
> > goto failure;
> > }
> >
> > If the virtualDev parameter is NULL, then we'd issue the
> > diagnostic and take the "goto", and (below), dereference NULL.
> >
> > >From 79283ba1d667534175d4c48079e6b500feba6480 Mon Sep 17 00:00:00 2001
> > From: Jim Meyering <meyering(a)redhat.com>
> > Date: Tue, 15 Dec 2009 16:07:10 +0100
> > Subject: [PATCH] esx_vmx.c: don't dereference NULL for a NULL virtualDev
> >
> > * src/esx/esx_vmx.c (esxVMX_ParseSCSIController): Don't deref
> > "virtualDev" when it is NULL.
> > ---
> > src/esx/esx_vmx.c | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/src/esx/esx_vmx.c b/src/esx/esx_vmx.c
> > index f5b4544..404617e 100644
> > --- a/src/esx/esx_vmx.c
> > +++ b/src/esx/esx_vmx.c
> > @@ -1204,7 +1204,8 @@ esxVMX_ParseSCSIController(virConnectPtr conn, virConfPtr
conf, int controller,
> > return 0;
> >
> > failure:
> > - VIR_FREE(*virtualDev);
> > + if (virtualDev)
> > + VIR_FREE(*virtualDev);
> >
> > return -1;
> > }
> > --
> > 1.6.6.rc2.275.g51e2d
> >
>
> This fixes the problem, but I would fix it differently, matching the
> other functions. See attached patch.
>
> Matthias
> commit 871cd31924308f963afd4df3834b3a1f354a5f8b
> Author: Matthias Bolte <matthias.bolte(a)googlemail.com>
> Date: Tue Dec 15 16:37:19 2009 +0100
>
> esx: Don't goto failure for invalid arguments
>
> This also fixes a NULL-deref of virtualDev in esxVMX_ParseSCSIController
> found by Jim Meyering.
>
> diff --git a/src/esx/esx_vmx.c b/src/esx/esx_vmx.c
> index f5b4544..7967718 100644
> --- a/src/esx/esx_vmx.c
> +++ b/src/esx/esx_vmx.c
> @@ -1165,14 +1165,14 @@ esxVMX_ParseSCSIController(virConnectPtr conn, virConfPtr
conf, int controller,
>
> if (virtualDev == NULL || *virtualDev != NULL) {
> ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Invalid argument");
> - goto failure;
> + return -1;
> }
>
> if (controller < 0 || controller > 3) {
> ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> "SCSI controller index %d out of [0..3] range",
> controller);
> - goto failure;
> + return -1;
> }
>
> snprintf(present_name, sizeof(present_name), "scsi%d.present",
controller);
> @@ -1642,7 +1642,7 @@ esxVMX_ParseEthernet(virConnectPtr conn, virConfPtr conf, int
controller,
> ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> "Ethernet controller index %d out of [0..3] range",
> controller);
> - goto failure;
> + return -1;
> }
>
> if (VIR_ALLOC(*def) < 0) {
> @@ -1840,7 +1840,7 @@ esxVMX_ParseSerial(virConnectPtr conn, esxVI_Context *ctx,
virConfPtr conf,
> if (port < 0 || port > 3) {
> ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> "Serial port index %d out of [0..3] range", port);
> - goto failure;
> + return -1;
> }
>
> if (VIR_ALLOC(*def) < 0) {
> @@ -1964,7 +1964,7 @@ esxVMX_ParseParallel(virConnectPtr conn, esxVI_Context *ctx,
virConfPtr conf,
> if (port < 0 || port > 2) {
> ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> "Parallel port index %d out of [0..2] range", port);
> - goto failure;
> + return -1;
> }
>
> if (VIR_ALLOC(*def) < 0) {
ACK
Daniel
Okay, pushed.
Matthias