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

Re: [libvirt] [PATCH] node_device_linux_sysfs.c: avoid opendir/fd leak on error path



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


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