[libvirt] [PATCH] esx_util.c: avoid NULL deref for invalid inputs

[Matthias Bolte]

2009/12/16 Jim Meyering <jim at meyering.net>:
> Jim Meyering wrote:
>>>>    if (sscanf(datastoreRelatedPath, "[%a[^]%]] %a[^\n]", datastoreName,
>>>>               &directoryAndFileName) != 2) {
> ...
> I suppose you know that %a[...] is non-standard (it's a glibc-specific
> addition) and that this esx code need only compile on systems with glibc.

No, I wasn't aware of that. The ESX driver is a libvirt client side
driver and should be compilable on non-linux platforms too. I think I
can replace the two sscanf calls with some strchr/strstr and strndup
calls.

> I confirm that with my change that would have leaked.
> Though note that it would do so only when parsing exactly one token.
>
>>> This fix is not correct, it may leak memory allocated by sscanf. goto
>>> failure is correct and safe here, because the first if block in this
>>> function has check the char ** pointers to be not-NULL, so the
>>> VIR_FREEs after the failure label is safe.
>>
>> Good catch.
>> I'll adjust and resend.
>
> I've folded in this correction:
>
> diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
> index 8703ac2..35a48e0 100644
> --- a/src/esx/esx_util.c
> +++ b/src/esx/esx_util.c
> @@ -299,7 +299,7 @@ esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn,
>         ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
>                   "Datastore related path '%s' doesn't have expected format "
>                   "'[<datastore>] <path>'", datastoreRelatedPath);
> -        return -1;
> +        goto failure;
>     }
>
>     /* Split <path> into <directory>/<file>, where <directory> is optional */
>
> Here's the result:
>
> From f34c4395b9b16965705f9158ac90e59c37b04507 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering at redhat.com>
> Date: Tue, 15 Dec 2009 19:08:49 +0100
> Subject: [PATCH] esx_util.c: avoid NULL deref for invalid inputs
>
> * src/esx/esx_util.c (esxUtil_ParseDatastoreRelatedPath): Return
> right away for invalid inputs, rather than using them (which would
> dereference NULL pointers) in clean-up code.
> ---
>  src/esx/esx_util.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
> index 3e53921..35a48e0 100644
> --- a/src/esx/esx_util.c
> +++ b/src/esx/esx_util.c
> @@ -277,7 +277,7 @@ esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn,
>         directoryName == NULL || *directoryName != NULL ||
>         fileName == NULL || *fileName != NULL) {
>         ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Invalid argument");
> -        goto failure;
> +        return -1;
>     }
>
>     /*
> --
> 1.6.6.rc2.275.g51e2d
>

ACK.

Matthias