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

[Matthias Bolte]

2009/12/15 Jim Meyering <jim at meyering.net>:
> The trouble here is that "goto failure" would run this code:
>
>  failure:
>    VIR_FREE(*datastoreName);
>    VIR_FREE(*directoryName);
>    VIR_FREE(*fileName);
>    result = -1;
>    goto cleanup;
>
> And thus if any of those 3 input variables was NULL,
> that deref would probably provoke a segfault.
>
> By the way, that interface should be changed,
> because it encourages risky behavior:
>
>  int
>  esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn,
>                                    const char *datastoreRelatedPath,
>                                    char **datastoreName,
>                                    char **directoryName, char **fileName)
>
> Note how it takes a buffer, datastoreRelatedPath,
> but with no length parameter.  Then this function
> writes into the buffer with this code:
>
>    if (sscanf(datastoreRelatedPath, "[%a[^]%]] %a[^\n]", datastoreName,
>               &directoryAndFileName) != 2) {

No, it doesn't write, it's sscanf and not printf. It reads from
datastoreRelatedPath.

> which cannot even check for buffer overrun because it doesn't
> know the length.
>
> You might want to add a new parameter, buf_len:
>
>  int
>  esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn,
>                                    const char *datastoreRelatedPath,
>                                    size_t buf_len,
>                                    char **datastoreName,
>                                    char **directoryName, char **fileName)
>
> >From aba304e07835c123bd63f1d5d6777a898916349f 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, rather than trying to clean up and dereference NULL
> pointers, if any input is invalid.
> ---
>  src/esx/esx_util.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
> index 3e53921..8703ac2 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;
>     }

Another one of this pattern. Okay.

>     /*
> @@ -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);
> -        goto failure;
> +        return -1;
>     }

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.

>     /* Split <path> into <directory>/<file>, where <directory> is optional */
> --
> 1.6.6.rc2.275.g51e2d
>

Partly NACK.

Thanks for finding this problems!

Matthias