[libvirt] [PATCH] usbFindBusByVendor: don't leak a DIR buffer and FD

[Jim Meyering]

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 at 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