4:13 a.m.
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(a)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
4:17 a.m.
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(a)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 :|
4:21 a.m.
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(a)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.
4:28 a.m.
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(a)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 :|
4:35 a.m.
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(a)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(a)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
5744
days inactive
5744
days old
4 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Jim Meyering