[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]