9:37 a.m.
I know better than to say this is so simple/obvious that
it doesn't need review, but I'm tempted...
From 02c7dfa830544bbafd62867fc70b3aba7cc07385 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 25 Jan 2010 16:36:07 +0100
Subject: [PATCH] usbFindBusByVendor: don't leak a DIR buffer and FD
* src/util/hostusb.c (usbFindBusByVendor): Don't leak a DIR buffer
and file descriptor.
---
src/util/hostusb.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/util/hostusb.c b/src/util/hostusb.c
index 8fbb486..9a37103 100644
--- a/src/util/hostusb.c
+++ b/src/util/hostusb.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -151,6 +151,7 @@ static int usbFindBusByVendor(virConnectPtr conn,
ret = 0;
error:
+ closedir (dir);
return ret;
}
--
1.7.0.rc0.127.gab8271
9:47 a.m.
On Mon, Jan 25, 2010 at 04:37:43PM +0100, Jim Meyering wrote:
I know better than to say this is so simple/obvious that
it doesn't need review, but I'm tempted...
>From 02c7dfa830544bbafd62867fc70b3aba7cc07385 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 25 Jan 2010 16:36:07 +0100
Subject: [PATCH] usbFindBusByVendor: don't leak a DIR buffer and FD
* src/util/hostusb.c (usbFindBusByVendor): Don't leak a DIR buffer
and file descriptor.
---
src/util/hostusb.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/util/hostusb.c b/src/util/hostusb.c
index 8fbb486..9a37103 100644
--- a/src/util/hostusb.c
+++ b/src/util/hostusb.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -151,6 +151,7 @@ static int usbFindBusByVendor(virConnectPtr conn,
ret = 0;
error:
+ closedir (dir);
return ret;
}
ACK, that context is a little misleading because the original code 'error:'
should have been 'cleanup:' really, since it is a shared success/failure
path, not a separate error path.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:55 a.m.
Daniel P. Berrange wrote:
On Mon, Jan 25, 2010 at 04:37:43PM +0100, Jim Meyering wrote:
> I know better than to say this is so simple/obvious that
> it doesn't need review, but I'm tempted...
>
> >From 02c7dfa830544bbafd62867fc70b3aba7cc07385 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering(a)redhat.com>
> Date: Mon, 25 Jan 2010 16:36:07 +0100
> Subject: [PATCH] usbFindBusByVendor: don't leak a DIR buffer and FD
>
> * src/util/hostusb.c (usbFindBusByVendor): Don't leak a DIR buffer
> and file descriptor.
> ---
> src/util/hostusb.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/src/util/hostusb.c b/src/util/hostusb.c
> index 8fbb486..9a37103 100644
> --- a/src/util/hostusb.c
> +++ b/src/util/hostusb.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2009 Red Hat, Inc.
> + * Copyright (C) 2009-2010 Red Hat, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -151,6 +151,7 @@ static int usbFindBusByVendor(virConnectPtr conn,
> ret = 0;
>
> error:
> + closedir (dir);
> return ret;
> }
ACK, that context is a little misleading because the original code 'error:'
should have been 'cleanup:' really, since it is a shared success/failure
path, not a separate error path.
I've pushed the above.
While doing that, I realized that closedir(NULL) might not go down well.
This fixes that and renames the labels in two functions.
From 351cb1fadf1e02d4b1a6f00bfadd9d992b2eb75c Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 25 Jan 2010 16:54:06 +0100
Subject: [PATCH] hostusb: closedir only if non-NULL; rename labels: s/error/cleanup/
* src/util/hostusb.c (usbSysReadFile): Rename labels s/error/cleanup/
(usbFindBusByVendor): Likewise. And closedir only if non-NULL.
---
src/util/hostusb.c | 23 ++++++++++++-----------
1 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/src/util/hostusb.c b/src/util/hostusb.c
index 9a37103..a452abd 100644
--- a/src/util/hostusb.c
+++ b/src/util/hostusb.c
@@ -70,20 +70,20 @@ static int usbSysReadFile(virConnectPtr conn,
tmp = virAsprintf(&filename, USB_SYSFS "/devices/%s/%s", d_name,
f_name);
if (tmp < 0) {
virReportOOMError(conn);
- goto error;
+ goto cleanup;
}
if (virFileReadAll(filename, 1024, &buf) < 0)
- goto error;
+ goto cleanup;
if (virStrToLong_ui(buf, &ignore, base, value) < 0) {
usbReportError(conn, VIR_ERR_INTERNAL_ERROR,
_("Could not parse usb file %s"), filename);
- goto error;
+ goto cleanup;
}
ret = 0;
-error:
+cleanup:
VIR_FREE(filename);
VIR_FREE(buf);
return ret;
@@ -103,7 +103,7 @@ static int usbFindBusByVendor(virConnectPtr conn,
virReportSystemError(conn, errno,
_("Could not open directory %s"),
USB_SYSFS "/devices");
- goto error;
+ goto cleanup;
}
while ((de = readdir(dir))) {
@@ -113,10 +113,10 @@ static int usbFindBusByVendor(virConnectPtr conn,
if (usbSysReadFile(conn, "idVendor", de->d_name,
16, &found_vend) < 0)
- goto error;
+ goto cleanup;
if (usbSysReadFile(conn, "idProduct", de->d_name,
16, &found_prod) < 0)
- goto error;
+ goto cleanup;
if (found_prod == product && found_vend == vendor) {
/* Lookup bus.addr info */
@@ -130,12 +130,12 @@ static int usbFindBusByVendor(virConnectPtr conn,
usbReportError(conn, VIR_ERR_INTERNAL_ERROR,
_("Failed to parse dir name '%s'"),
de->d_name);
- goto error;
+ goto cleanup;
}
if (usbSysReadFile(conn, "devnum", de->d_name,
10, &found_addr) < 0)
- goto error;
+ goto cleanup;
*bus = found_bus;
*devno = found_addr;
@@ -150,8 +150,9 @@ static int usbFindBusByVendor(virConnectPtr conn,
else
ret = 0;
-error:
- closedir (dir);
+cleanup:
+ if (dir)
+ closedir (dir);
return ret;
}
--
1.7.0.rc0.127.gab8271
7:29 a.m.
According to Jim Meyering on 1/25/2010 8:55 AM:
Subject: [PATCH] hostusb: closedir only if non-NULL; rename labels:
s/error/cleanup/
* src/util/hostusb.c (usbSysReadFile): Rename labels s/error/cleanup/
(usbFindBusByVendor): Likewise. And closedir only if non-NULL.
@@ -150,8 +150,9 @@ static int usbFindBusByVendor(virConnectPtr conn,
else
ret = 0;
-error:
- closedir (dir);
+cleanup:
+ if (dir)
+ closedir (dir);
Should errno be saved and restored around this point, to avoid it being
arbitrarily changed by closedir?
--
Don't work too hard, make some time for fun as well!
Eric Blake ebb9(a)byu.net
8:59 a.m.
Eric Blake wrote:
According to Jim Meyering on 1/25/2010 8:55 AM:
> Subject: [PATCH] hostusb: closedir only if non-NULL; rename labels: s/error/cleanup/
>
> * src/util/hostusb.c (usbSysReadFile): Rename labels s/error/cleanup/
> (usbFindBusByVendor): Likewise. And closedir only if non-NULL.
> @@ -150,8 +150,9 @@ static int usbFindBusByVendor(virConnectPtr conn,
> else
> ret = 0;
>
> -error:
> - closedir (dir);
> +cleanup:
> + if (dir)
> + closedir (dir);
Should errno be saved and restored around this point, to avoid it being
arbitrarily changed by closedir?
Thanks for looking.
Technically you're right, and I'll merge this:
diff --git a/src/util/hostusb.c b/src/util/hostusb.c
index 72d0833..71f6435 100644
--- a/src/util/hostusb.c
+++ b/src/util/hostusb.c
@@ -151,8 +151,11 @@ static int usbFindBusByVendor(virConnectPtr conn,
ret = 0;
cleanup:
- if (dir)
+ if (dir) {
+ int saved_errno = errno;
closedir (dir);
+ errno = saved_errno;
+ }
return ret;
}
However, no caller that I have seen uses errno (I looked at all
direct callers and stopped after getting to 4th or 5th-level indirect
callers in one part of the call tree), so currently it makes no difference.
Even if a caller did use errno, it'd be unusable some of the
time because this function calls usbSysReadFile, which also
clobbers errno via its free-upon-cleanup calls.
BTW, I noticed that this function treats a failed readdir
just like end-of-directory (i.e., ignores it), but I didn't
bother to fix that. There are far bigger fish to fry, currently.
But we'll have to start someday.
9:04 a.m.
On Tue, Jan 26, 2010 at 03:59:08PM +0100, Jim Meyering wrote:
Eric Blake wrote:
> According to Jim Meyering on 1/25/2010 8:55 AM:
>> Subject: [PATCH] hostusb: closedir only if non-NULL; rename labels:
s/error/cleanup/
>>
>> * src/util/hostusb.c (usbSysReadFile): Rename labels s/error/cleanup/
>> (usbFindBusByVendor): Likewise. And closedir only if non-NULL.
>> @@ -150,8 +150,9 @@ static int usbFindBusByVendor(virConnectPtr conn,
>> else
>> ret = 0;
>>
>> -error:
>> - closedir (dir);
>> +cleanup:
>> + if (dir)
>> + closedir (dir);
>
> Should errno be saved and restored around this point, to avoid it being
> arbitrarily changed by closedir?
Thanks for looking.
Technically you're right, and I'll merge this:
diff --git a/src/util/hostusb.c b/src/util/hostusb.c
index 72d0833..71f6435 100644
--- a/src/util/hostusb.c
+++ b/src/util/hostusb.c
@@ -151,8 +151,11 @@ static int usbFindBusByVendor(virConnectPtr conn,
ret = 0;
cleanup:
- if (dir)
+ if (dir) {
+ int saved_errno = errno;
closedir (dir);
+ errno = saved_errno;
+ }
return ret;
}
However, no caller that I have seen uses errno (I looked at all
direct callers and stopped after getting to 4th or 5th-level indirect
callers in one part of the call tree), so currently it makes no difference.
Yep, generally speaking if a caller needs to be given back the actual
errno value, then the function should be returing '-errno' instead
of the fixed -1.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
10:10 a.m.
Daniel P. Berrange wrote:
On Tue, Jan 26, 2010 at 03:59:08PM +0100, Jim Meyering wrote:
> Eric Blake wrote:
>
> > According to Jim Meyering on 1/25/2010 8:55 AM:
> >> Subject: [PATCH] hostusb: closedir only if non-NULL; rename labels:
s/error/cleanup/
> >>
> >> * src/util/hostusb.c (usbSysReadFile): Rename labels s/error/cleanup/
> >> (usbFindBusByVendor): Likewise. And closedir only if non-NULL.
> >> @@ -150,8 +150,9 @@ static int usbFindBusByVendor(virConnectPtr conn,
> >> else
> >> ret = 0;
> >>
> >> -error:
> >> - closedir (dir);
> >> +cleanup:
> >> + if (dir)
> >> + closedir (dir);
> >
> > Should errno be saved and restored around this point, to avoid it being
> > arbitrarily changed by closedir?
>
> Thanks for looking.
> Technically you're right, and I'll merge this:
>
> diff --git a/src/util/hostusb.c b/src/util/hostusb.c
> index 72d0833..71f6435 100644
> --- a/src/util/hostusb.c
> +++ b/src/util/hostusb.c
> @@ -151,8 +151,11 @@ static int usbFindBusByVendor(virConnectPtr conn,
> ret = 0;
>
> cleanup:
> - if (dir)
> + if (dir) {
> + int saved_errno = errno;
> closedir (dir);
> + errno = saved_errno;
> + }
> return ret;
> }
>
> However, no caller that I have seen uses errno (I looked at all
> direct callers and stopped after getting to 4th or 5th-level indirect
> callers in one part of the call tree), so currently it makes no difference.
Yep, generally speaking if a caller needs to be given back the actual
errno value, then the function should be returing '-errno' instead
of the fixed -1.
FYI, I've pushed this:
From fb54230b60be19ccdad39fd723d7d75f3a920808 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 25 Jan 2010 16:54:06 +0100
Subject: [PATCH] hostusb: closedir only if non-NULL; rename labels: s/error/cleanup/
* src/util/hostusb.c (usbSysReadFile): Rename labels s/error/cleanup/
(usbFindBusByVendor): Likewise. And closedir only if non-NULL.
---
src/util/hostusb.c | 26 +++++++++++++++-----------
1 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/src/util/hostusb.c b/src/util/hostusb.c
index f635ce5..71f6435 100644
--- a/src/util/hostusb.c
+++ b/src/util/hostusb.c
@@ -70,20 +70,20 @@ static int usbSysReadFile(virConnectPtr conn,
tmp = virAsprintf(&filename, USB_SYSFS "/devices/%s/%s", d_name,
f_name);
if (tmp < 0) {
virReportOOMError(conn);
- goto error;
+ goto cleanup;
}
if (virFileReadAll(filename, 1024, &buf) < 0)
- goto error;
+ goto cleanup;
if (virStrToLong_ui(buf, &ignore, base, value) < 0) {
usbReportError(conn, VIR_ERR_INTERNAL_ERROR,
_("Could not parse usb file %s"), filename);
- goto error;
+ goto cleanup;
}
ret = 0;
-error:
+cleanup:
VIR_FREE(filename);
VIR_FREE(buf);
return ret;
@@ -103,7 +103,7 @@ static int usbFindBusByVendor(virConnectPtr conn,
virReportSystemError(conn, errno,
_("Could not open directory %s"),
USB_SYSFS "/devices");
- goto error;
+ goto cleanup;
}
while ((de = readdir(dir))) {
@@ -113,10 +113,10 @@ static int usbFindBusByVendor(virConnectPtr conn,
if (usbSysReadFile(conn, "idVendor", de->d_name,
16, &found_vend) < 0)
- goto error;
+ goto cleanup;
if (usbSysReadFile(conn, "idProduct", de->d_name,
16, &found_prod) < 0)
- goto error;
+ goto cleanup;
if (found_prod == product && found_vend == vendor) {
/* Lookup bus.addr info */
@@ -130,12 +130,12 @@ static int usbFindBusByVendor(virConnectPtr conn,
usbReportError(conn, VIR_ERR_INTERNAL_ERROR,
_("Failed to parse dir name '%s'"),
de->d_name);
- goto error;
+ goto cleanup;
}
if (usbSysReadFile(conn, "devnum", de->d_name,
10, &found_addr) < 0)
- goto error;
+ goto cleanup;
*bus = found_bus;
*devno = found_addr;
@@ -150,8 +150,12 @@ static int usbFindBusByVendor(virConnectPtr conn,
else
ret = 0;
-error:
- closedir (dir);
+cleanup:
+ if (dir) {
+ int saved_errno = errno;
+ closedir (dir);
+ errno = saved_errno;
+ }
return ret;
}
--
1.7.0.rc0.140.gfbe7
12:19 p.m.
On 01/26/2010 10:04 AM, Daniel P. Berrange wrote:
Yep, generally speaking if a caller needs to be given back the
actual
errno value, then the function should be returing '-errno' instead
of the fixed -1.
-errno? Interesting. I recently fixed calls to virFileMakePath which
were looking for return < 0 to look for return != 0, because it returns
the value of errno (not -errno) in case of an error. Should I have
changed virFileMakePath to return -errno instead? All of the negations
through different paths in multiple layers might start to get confusing...
1:04 p.m.
On Tue, Jan 26, 2010 at 01:19:18PM -0500, Laine Stump wrote:
On 01/26/2010 10:04 AM, Daniel P. Berrange wrote:
>Yep, generally speaking if a caller needs to be given back the actual
>errno value, then the function should be returing '-errno' instead
>of the fixed -1.
>
-errno? Interesting. I recently fixed calls to virFileMakePath which
were looking for return < 0 to look for return != 0, because it returns
the value of errno (not -errno) in case of an error. Should I have
changed virFileMakePath to return -errno instead? All of the negations
through different paths in multiple layers might start to get confusing...
To be fair, it isn't actually clearcut one way or the other at this time.
There's a fairly even balance between functions returning 'errno' and
'-errno'. 95% of them just return '-1' though, and report errors directly
via one of virReportErrorXXXX functions. We should only return errno in
cases where the caller needs to be able to see and ignore the failure
scenarios.
Since 95% of cases use the simple 'return -1' for errors, my preference
is that we use 'return -errno' too, since that means we have a fairly
standard pattern of
if (foo() < 0) {
...blah...
}
or
if ((err = foo()) < 0) {
...blah...
}
for detecting errors when 'int' is the return type.
We should probably document this in the HACKING file and then change
an exceptions to match, but its not urgent.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:52 a.m.
On Mon, Jan 25, 2010 at 04:37:43PM +0100, Jim Meyering wrote:
I know better than to say this is so simple/obvious that
it doesn't need review, but I'm tempted...
>From 02c7dfa830544bbafd62867fc70b3aba7cc07385 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 25 Jan 2010 16:36:07 +0100
Subject: [PATCH] usbFindBusByVendor: don't leak a DIR buffer and FD
* src/util/hostusb.c (usbFindBusByVendor): Don't leak a DIR buffer
and file descriptor.
---
src/util/hostusb.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/util/hostusb.c b/src/util/hostusb.c
index 8fbb486..9a37103 100644
--- a/src/util/hostusb.c
+++ b/src/util/hostusb.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -151,6 +151,7 @@ static int usbFindBusByVendor(virConnectPtr conn,
ret = 0;
error:
+ closedir (dir);
return ret;
}
Ah right ! ACK
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5414
days inactive
5415
days old
9 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Jim Meyering -
Laine Stump