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

[Jim Meyering]

Matthias Bolte wrote:
> 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.

Whoops ;-)
Shame on me.

>> 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.

Good catch.
I'll adjust and resend.