2009/12/15 Jim Meyering <jim(a)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(a)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