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

[Dave Allan]

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 at 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 at 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 at 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