[libvirt] [PATCH] node_device_linux_sysfs.c: avoid opendir/fd leak on error path
This looks pretty clear. It calls closedir upon successful return, but not in the error case.
From 526a2bc2d87f0bf7a0c16a2a96316e5c1d5dae2e Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 20 Jan 2010 17:49:35 +0100 Subject: [PATCH] node_device_linux_sysfs.c: avoid opendir/fd leak on error path
* src/node_device/node_device_linux_sysfs.c(get_virtual_functions_linux): Don't leak a DIR buffer and file descriptor on error path. --- src/node_device/node_device_linux_sysfs.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index ff7aaf0..e611e1a 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -2,7 +2,7 @@ * node_device_hal_linuc.c: Linux specific code to gather device data * not available through HAL. * - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -371,6 +371,8 @@ int get_virtual_functions_linux(const char *sysfs_path, ret = 0; out: + if (dir) + closedir(dir); VIR_FREE(device_link); return 0; } -- 1.6.6.516.gb72f
On 01/20/2010 11:52 AM, Jim Meyering wrote:
This looks pretty clear. It calls closedir upon successful return, but not in the error case.
From 526a2bc2d87f0bf7a0c16a2a96316e5c1d5dae2e Mon Sep 17 00:00:00 2001 From: Jim Meyering<meyering@redhat.com> Date: Wed, 20 Jan 2010 17:49:35 +0100 Subject: [PATCH] node_device_linux_sysfs.c: avoid opendir/fd leak on error path
* src/node_device/node_device_linux_sysfs.c(get_virtual_functions_linux): Don't leak a DIR buffer and file descriptor on error path. --- src/node_device/node_device_linux_sysfs.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index ff7aaf0..e611e1a 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -2,7 +2,7 @@ * node_device_hal_linuc.c: Linux specific code to gather device data * not available through HAL. * - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -371,6 +371,8 @@ int get_virtual_functions_linux(const char *sysfs_path, ret = 0;
out: + if (dir) + closedir(dir); VIR_FREE(device_link); return 0; } -- 1.6.6.516.gb72f
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
ACK Dave
Jim Meyering wrote:
This looks pretty clear. It calls closedir upon successful return, but not in the error case.
From 526a2bc2d87f0bf7a0c16a2a96316e5c1d5dae2e Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 20 Jan 2010 17:49:35 +0100 Subject: [PATCH] node_device_linux_sysfs.c: avoid opendir/fd leak on error path
* src/node_device/node_device_linux_sysfs.c(get_virtual_functions_linux): Don't leak a DIR buffer and file descriptor on error path. --- src/node_device/node_device_linux_sysfs.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index ff7aaf0..e611e1a 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -2,7 +2,7 @@ * node_device_hal_linuc.c: Linux specific code to gather device data * not available through HAL. * - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -371,6 +371,8 @@ int get_virtual_functions_linux(const char *sysfs_path, ret = 0;
out: + if (dir) + closedir(dir); VIR_FREE(device_link); return 0; }
That patch is incomplete. Would have been obvious if I'd included a few more lines of context above that "out:" label: closedir(dir); ret = 0; It lacked a last-minute addition that is required. I'll fold this in: diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index e611e1a..674ee26 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -367,6 +367,7 @@ int get_virtual_functions_linux(const char *sysfs_path, } closedir(dir); + dir = NULL; ret = 0; Here's the revised patch:
From a331c0194def0fe24f8b40d4ef91525eacff4cfb Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 20 Jan 2010 17:49:35 +0100 Subject: [PATCH] node_device_linux_sysfs.c: avoid opendir/fd leak on error path
* src/node_device/node_device_linux_sysfs.c(get_virtual_functions_linux): Don't leak a DIR buffer and file descriptor on error path. --- src/node_device/node_device_linux_sysfs.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index ff7aaf0..674ee26 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -2,7 +2,7 @@ * node_device_hal_linuc.c: Linux specific code to gather device data * not available through HAL. * - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -367,10 +367,13 @@ int get_virtual_functions_linux(const char *sysfs_path, } closedir(dir); + dir = NULL; ret = 0; out: + if (dir) + closedir(dir); VIR_FREE(device_link); return 0; } -- 1.6.6.516.gb72f
On 01/20/2010 12:46 PM, Jim Meyering wrote:
Jim Meyering wrote:
This looks pretty clear. It calls closedir upon successful return, but not in the error case.
From 526a2bc2d87f0bf7a0c16a2a96316e5c1d5dae2e Mon Sep 17 00:00:00 2001 From: Jim Meyering<meyering@redhat.com> Date: Wed, 20 Jan 2010 17:49:35 +0100 Subject: [PATCH] node_device_linux_sysfs.c: avoid opendir/fd leak on error path
* src/node_device/node_device_linux_sysfs.c(get_virtual_functions_linux): Don't leak a DIR buffer and file descriptor on error path. --- src/node_device/node_device_linux_sysfs.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index ff7aaf0..e611e1a 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -2,7 +2,7 @@ * node_device_hal_linuc.c: Linux specific code to gather device data * not available through HAL. * - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -371,6 +371,8 @@ int get_virtual_functions_linux(const char *sysfs_path, ret = 0;
out: + if (dir) + closedir(dir); VIR_FREE(device_link); return 0; }
That patch is incomplete. Would have been obvious if I'd included a few more lines of context above that "out:" label:
closedir(dir); ret = 0;
It lacked a last-minute addition that is required. I'll fold this in:
diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index e611e1a..674ee26 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -367,6 +367,7 @@ int get_virtual_functions_linux(const char *sysfs_path, }
closedir(dir); + dir = NULL;
ret = 0;
Here's the revised patch:
From a331c0194def0fe24f8b40d4ef91525eacff4cfb Mon Sep 17 00:00:00 2001 From: Jim Meyering<meyering@redhat.com> Date: Wed, 20 Jan 2010 17:49:35 +0100 Subject: [PATCH] node_device_linux_sysfs.c: avoid opendir/fd leak on error path
* src/node_device/node_device_linux_sysfs.c(get_virtual_functions_linux): Don't leak a DIR buffer and file descriptor on error path. --- src/node_device/node_device_linux_sysfs.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index ff7aaf0..674ee26 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -2,7 +2,7 @@ * node_device_hal_linuc.c: Linux specific code to gather device data * not available through HAL. * - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -367,10 +367,13 @@ int get_virtual_functions_linux(const char *sysfs_path, }
closedir(dir); + dir = NULL;
ret = 0;
out: + if (dir) + closedir(dir); VIR_FREE(device_link); return 0; } -- 1.6.6.516.gb72f
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Since dir is initialized to NULL, you could also simplify things by removing the closedir/dir = NULL statements above the out: label. Either way, this version looks safe to me. Dave
Dave Allan wrote: ...
closedir(dir); + dir = NULL;
ret = 0;
out: + if (dir) + closedir(dir); VIR_FREE(device_link); return 0; }
... Since dir is initialized to NULL, you could also simplify things by removing the closedir/dir = NULL statements above the out: label. Either way, this version looks safe to me.
Good idea. I've just done that, too: commit 21c84e00d3c094abc5e429eec83c6fb5b2e25695 Author: Jim Meyering <meyering@redhat.com> Date: Wed Jan 20 21:57:34 2010 +0100 clean-up: remove unnecessary closedir call * src/node_device/node_device_linux_sysfs.c (get_virtual_functions_linux): Remove unnecessary closedir. Spotted by Dave Allan. diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 674ee26..73d8f8e 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -362,17 +362,14 @@ int get_virtual_functions_linux(const char *sysfs_path, d->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; } VIR_FREE(device_link); } } - closedir(dir); - dir = NULL; - ret = 0; out: if (dir) closedir(dir); VIR_FREE(device_link); return 0;
participants (2)
-
Dave Allan -
Jim Meyering