[libvirt] [PATCH] don't leak a file descriptor on failed pciGetDevice call

[Jim Meyering]

Daniel P. Berrange wrote:

> On Tue, Mar 03, 2009 at 11:21:16AM +0100, Jim Meyering wrote:
>> Daniel P. Berrange wrote:
>> > On Tue, Mar 03, 2009 at 11:13:40AM +0100, Jim Meyering wrote:
>> >> This loop would mistakenly return early (skipping the closedir)
>> >> upon pciGetDevice failure.
>> >>
>> >> >From 2d4d1d25edf8f1c3f4770707215bba67d73fd59f Mon Sep 17 00:00:00 2001
>> >> From: Jim Meyering <meyering at redhat.com>
>> >> Date: Tue, 3 Mar 2009 11:11:07 +0100
>> >> Subject: [PATCH] don't leak a file descriptor on failed pciGetDevice call
>> >
>> > ACK, but...
>> >
>> >> @@ -823,7 +827,7 @@ void
>> >>  pciFreeDevice(virConnectPtr conn ATTRIBUTE_UNUSED, pciDevice *dev)
>> >>  {
>> >>      VIR_DEBUG("%s %s: freeing", dev->id, dev->name);
>> >> -    if (dev->fd)
>> >> +    if (dev->fd >= 0)
>> >>          close(dev->fd);
>> >>      VIR_FREE(dev);
>> >>  }
>> >
>> > With this change you also need to fix the bug in pciGetDevice() which
>> > leaves 'fd' initialized to 0 rather than setting it to -1
>>
>> Ha.  Good catch.
>> That change did not belong in this patch (note lack of mention in log message).
>> It belongs in a separate patch.
>
> Yes it does because, previously it would be harmless due to
>
>   if (dev->fd)
>
> only check for fd > 0, but with this new patch applied it turns into a
> real bug, since the check is now fd >= 0.

There are two potential leaks.  Their patches should be separate.
This change

>> >> -    if (dev->fd)
>> >> +    if (dev->fd >= 0)

does not belong in the patch fixing the dir-handle leak.
I'll commit these in an hour or so.

Here's the separate patch:


>From 842ac8e23b365cc460d3e03733021bed406d8e67 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering at redhat.com>
Date: Tue, 3 Mar 2009 11:26:02 +0100
Subject: [PATCH] avoid file descriptor leak when fd == 0

* src/pci.c (pciGetDevice): Initialize dev->fd to -1, not 0.
(pciFreeDevice): Close fd also when it is 0.
---
 src/pci.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/pci.c b/src/pci.c
index 29a6dcc..b9c781b 100644
--- a/src/pci.c
+++ b/src/pci.c
@@ -789,6 +789,7 @@ pciGetDevice(virConnectPtr conn,
         return NULL;
     }

+    dev->fd       = -1;
     dev->domain   = domain;
     dev->bus      = bus;
     dev->slot     = slot;
@@ -827,7 +828,7 @@ void
 pciFreeDevice(virConnectPtr conn ATTRIBUTE_UNUSED, pciDevice *dev)
 {
     VIR_DEBUG("%s %s: freeing", dev->id, dev->name);
-    if (dev->fd)
+    if (dev->fd >= 0)
         close(dev->fd);
     VIR_FREE(dev);
 }
--
1.6.2.rc1.285.gc5f54