Matthias Bolte wrote:
> 2009/12/15 Jim Meyering <jim@xxxxxxxxxxxx>:
>> 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@xxxxxxxxxx>
>> 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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list