2009/8/14 Eduardo Otubo <otubo@xxxxxxxxxxxxxxxxxx>:
> On Fri, 2009-08-07 at 15:35 +0200, Chris Lalancette wrote:
>> Matthias Bolte wrote:
>> > Hi,
>> >
>> > I came across this line in the phypOpen function:
>> >
>> > char string[strlen(conn->uri->path)];
>> >
>> > Here the path part of the given URI is used without checking it for
>> > NULL, this can cause a segfault as strlen expects a string != NULL.
>>
>> Heh, it's worse than that; there is a check later on for !conn || !conn->uri, so
>> you are potentially de-referencing a NULL pointer.
>>
>> > Beside that uuid_db and connection_data leak in case of an error.
>> >
>> > In this line
>> >
>> > conn->uri->path = string;
>> >
>> > the original path of the URI leaks. The patch adds a VIR_FREE call
>> > before setting the new path.
>> >
>> > The attached patch is compile-tested but I don't have a Power
>> > Hypervisor installation at hand to test it for real.
>>
>> I also don't have a Power Hypervisor, but it looks sane enough to me. I'll say
>> ACK, but it's probably a good idea to get someone who has Power to test it
>> before you commit.
>>
>
> I tested with some Power machines I have over here and it is ACK for me.
>
> []'s
>
A change to escape_specialcharacters() affects this patch, so I
attached a v2 of it. The only change to the first patch is the caching
of strlen(conn->uri->path) + 1 to use it for VIR_ALLOC_N() and
escape_specialcharacters().
Matthias
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index f457cf4..9b46696 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -63,25 +63,18 @@ static virDrvOpenStatus
phypOpen(virConnectPtr conn,
virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED)
{
- SSH_SESSION *session;
- ConnectionData *connection_data;
- char string[strlen(conn->uri->path)];
-
+ SSH_SESSION *session = NULL;
+ ConnectionData *connection_data = NULL;
+ char *string = NULL;
+ size_t len = 0;
uuid_dbPtr uuid_db = NULL;
- if (VIR_ALLOC(uuid_db) < 0)
- virReportOOMError(conn);
-
- if (VIR_ALLOC(connection_data) < 0)
- virReportOOMError(conn);
-
if (!conn || !conn->uri)
return VIR_DRV_OPEN_DECLINED;
if (conn->uri->scheme == NULL || STRNEQ(conn->uri->scheme, "phyp"))
return VIR_DRV_OPEN_DECLINED;
-
if (conn->uri->server == NULL) {
virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP,
VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s",
@@ -96,20 +89,38 @@ phypOpen(virConnectPtr conn,
return VIR_DRV_OPEN_ERROR;
}
- if (escape_specialcharacters(conn->uri->path, string, sizeof(string)) == -1) {
+ if (VIR_ALLOC(uuid_db) < 0) {
+ virReportOOMError(conn);
+ goto failure;
+ }
+
+ if (VIR_ALLOC(connection_data) < 0) {
+ virReportOOMError(conn);
+ goto failure;
+ }
+
+ len = strlen(conn->uri->path) + 1;
+
+ if (VIR_ALLOC_N(string, len) < 0) {
+ virReportOOMError(conn);
+ goto failure;
+ }
+
+ if (escape_specialcharacters(conn->uri->path, string, len) == -1) {
virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP,
VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s",
_("Error parsing 'path'. Invalid characters."));
- return VIR_DRV_OPEN_ERROR;
+ goto failure;
}
if ((session = openSSHSession(conn, auth)) == NULL) {
virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP,
VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s",
_("Error while opening SSH session."));
- return VIR_DRV_OPEN_ERROR;
+ goto failure;
}
+ VIR_FREE(conn->uri->path);
conn->uri->path = string;
connection_data->session = session;
connection_data->auth = auth;
@@ -122,6 +133,13 @@ phypOpen(virConnectPtr conn,
init_uuid_db(conn);
return VIR_DRV_OPEN_SUCCESS;
+
+ failure:
+ VIR_FREE(uuid_db);
+ VIR_FREE(connection_data);
+ VIR_FREE(string);
+
+ return VIR_DRV_OPEN_ERROR;
}
static int
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list