[libvirt] [PATCH] usbFindBusByVendor: don't leak a DIR buffer and FD
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@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
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@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 :|
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@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@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
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@byu.net
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.
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 :|
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@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
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...
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 :|
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@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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Jim Meyering -
Laine Stump