[libvirt] [PATCH] don't leak a file descriptor on failed pciGetDevice call
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@redhat.com> Date: Tue, 3 Mar 2009 11:11:07 +0100 Subject: [PATCH] don't leak a file descriptor on failed pciGetDevice call
* src/pci.c (pciIterDevices): Always close dir handle. --- src/pci.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/pci.c b/src/pci.c index 2343be3..501c6aa 100644 --- a/src/pci.c +++ b/src/pci.c @@ -233,6 +233,7 @@ pciIterDevices(virConnectPtr conn, { DIR *dir; struct dirent *entry; + int ret = 0; *matched = NULL; @@ -252,14 +253,17 @@ pciIterDevices(virConnectPtr conn, if (entry->d_name[0] == '.') continue; - if (sscanf(entry->d_name, "%x:%x:%x.%x", &domain, &bus, &slot, &function) < 4) { + if (sscanf(entry->d_name, "%x:%x:%x.%x", + &domain, &bus, &slot, &function) < 4) { VIR_WARN("Unusual entry in " PCI_SYSFS "devices: %s", entry->d_name); continue; } try = pciGetDevice(conn, domain, bus, slot, function); - if (!try) - return -1; + if (!try) { + ret = -1; + break; + } if (predicate(try, dev)) { VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, try->name); @@ -269,7 +273,7 @@ pciIterDevices(virConnectPtr conn, pciFreeDevice(conn, try); } closedir(dir); - return 0; + return ret; } static uint8_t @@ -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); } -- 1.6.2.rc1.285.gc5f54
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@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 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, 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@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.
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@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. 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, 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@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@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
participants (2)
-
Daniel P. Berrange -
Jim Meyering