Re: [btrfs-progs] [bug][patch V2] Leaking file handle in scrub_fs_info()

[Goffredo Baroncelli <kreijack@libero.it>]

[-- Attachment #1: Type: text/plain, Size: 10246 bytes --]

Hi Hugo,

I was not able to reproduce your error with my repository: I pulled it
without problem.

However, enclosed you can find the patch (as attachment) with the Jan
suggestions about the spaces. You can pull the patch also from my repo (
same branch: fd-leaking). Moreover I inserted the patch as inlined.
Hoping that helps.

--------------------

commit b73abc9465d3a105b5f8f464f2867543a638e5e2
Author: Goffredo Baroncelli <kreijack@inwind.it>
Date:   Tue Jun 5 19:11:06 2012 +0200

 scrub_fs_info( ) file handle leaking

 The function scrub_fs_info( ) closes and reopen a file handle
 passed as argument, when a caller uses the file handle even after the
 call.
 The function scrub_fs_info( ) is updated to remove the file handle
 argument, and instead uses a private own file handle.
 The callers are updated to not pass the argument.


diff --git a/cmds-scrub.c b/cmds-scrub.c
index c4503f4..6313e02 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
        return ret ? -errno : 0;
 }

-static int scrub_fs_info(int fd, char *path,
+static int scrub_fs_info(char *path,
                                struct btrfs_ioctl_fs_info_args *fi_args,
                                struct btrfs_ioctl_dev_info_args **di_ret)
 {
        int ret = 0;
        int ndevs = 0;
        int i = 1;
+       int fd;
        struct btrfs_fs_devices *fs_devices_mnt = NULL;
        struct btrfs_ioctl_dev_info_args *di_args;
        char mp[BTRFS_PATH_NAME_MAX + 1];

        memset(fi_args, 0, sizeof(*fi_args));

+       fd  = open_file_or_dir(path);
+       if (fd < 0) {
+               fprintf(stderr, "ERROR: can't access to '%s'\n", path);
+               return -1;
+       }
+
        ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
        if (ret && errno == EINVAL) {
                /* path is no mounted btrfs. try if it's a device */
@@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
                if (fd < 0)
                        return -errno;
        } else if (ret) {
+               close(fd);
                return -errno;
        }

-       if (!fi_args->num_devices)
+       if (!fi_args->num_devices) {
+               close(fd);
                return 0;
+       }

        di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
-       if (!di_args)
+       if (!di_args) {
+               close(fd);
                return -errno;
+       }

        for (; i <= fi_args->max_id; ++i) {
                BUG_ON(ndevs >= fi_args->num_devices);
                ret = scrub_device_info(fd, i, &di_args[ndevs]);
                if (ret == -ENODEV)
                        continue;
-               if (ret)
+               if (ret) {
+                       close(fd);
                        return ret;
+               }
                ++ndevs;
        }

        BUG_ON(ndevs == 0);

+       close(fd);
        return 0;
 }

@@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int
resume)
                return 12;
        }

-       ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
+       ret = scrub_fs_info(path, &fi_args, &di_args);
        if (ret) {
                ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
                    "%s\n", strerror(-ret));
@@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
                .sun_family = AF_UNIX,
        };
        int ret;
-       int fdmnt;
        int i;
        int print_raw = 0;
        int do_stats_per_dev = 0;
@@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)

        path = argv[optind];

-       fdmnt = open_file_or_dir(path);
-       if (fdmnt < 0) {
-               fprintf(stderr, "ERROR: can't access to '%s'\n", path);
-               return 12;
-       }
-
-       ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
+       ret = scrub_fs_info(path, &fi_args, &di_args);
        if (ret) {
                fprintf(stderr, "ERROR: getting dev info for scrub failed: "
                                "%s\n", strerror(-ret));
@@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
 out:
        free_history(past_scrubs);
        free(di_args);
-       close(fdmnt);
        if (fdres > -1)
                close(fdres);






On 06/05/2012 01:01 PM, Hugo Mills wrote:
>    Hi, Goffredo,
> 
>    I'm trying to do a new integration branch, and the patch inline in
> the text is corrupt (plus it has a massively verbose commit message):
> 
> Applying: Leaking file handle in scrub_fs_info()
> fatal: corrupt patch at line 74
> Patch failed at 0001 Leaking file handle in scrub_fs_info()
> 
>    Normally, I'd use your git repo to get round the fact that every
> single patch you send by mail is whitespace-damaged, but:
> 
> error: Unable to find 337bfb3250203a66b953ffcf7804418cb7c72052 under http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
> Cannot obtain needed commit 337bfb3250203a66b953ffcf7804418cb7c72052
> while processing commit ee3832b47a4b1d1dce8d1cacc802db32901b8c45.
> error: Fetch failed.
> 
>    Plus Jan had some suggested tweaks regarding whitespace -- could
> you re-roll the patch and maybe see what you can do to fix your git
> repo? (Or at least use git-send-email to send the patch so it doesn't
> get mangled).
> 
>    Thanks,
>    Hugo.
> 
> On Tue, Apr 24, 2012 at 08:43:23PM +0200, Goffredo Baroncelli wrote:
>> I was giving a look to the function scrub_fs_info( ), and to me it seems
>> that could be a potential file handle leaking problem.
>>
>>
>> In fact:
>>
>> static int scrub_fs_info(int fd, char *path,
>>                     struct btrfs_ioctl_fs_info_args *fi_args,
>>                     struct btrfs_ioctl_dev_info_args **di_ret)
>> {
>>
>> [...]
>>
>>         ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
>>         if (ret && errno == EINVAL) {
>>                 /* path is no mounted btrfs. try if it's a device */
>> [...]
>>                 close(fd);			<--- Here the
>> 						     file handle is
>> 						     closed
>>
>>                 fd = open_file_or_dir(mp);	<--- then it is
>> 						     re-opened
>>                 if (fd < 0)
>>                         return -errno;
>>         } else if (ret) {
>>                 return -errno;
>>         }
>> [...]
>>
>> But in the rest of the function:
>> a) the file handle is not closed
>> b) the (new) file handle isn't returned
>>
>> The function "scrub_fs_info()" is called from the functions
>> 1) cmd_scrub_status(), which doesn't use the file handle after the call
>> to the cmd_scrub_status() [except for a close()]. So no problem at all.
>> 2) scrub_start(), which uses the file handle after the call to the
>> cmd_scrub_status() functions.
>>
>> My suggestions is to change scrub_fs_info() to accept only the path.
>> Then it open (and closes) its own (and private) the file descriptor.
>>
>> Instead scrub_start(), opens a file descriptor after the call to the
>> scrub_fs_info() function.
>>
>> What do you think ?
>>
>> BR
>> G.Baroncelli
>>
>> You can pull the patch below from
>>
>> 	http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
>>
>> branch
>>
>> 	fd-leaking
>>
>> -----
>>
>> diff --git a/cmds-scrub.c b/cmds-scrub.c
>> index c4503f4..486768c 100644
>> --- a/cmds-scrub.c
>> +++ b/cmds-scrub.c
>> @@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
>>  	return ret ? -errno : 0;
>>  }
>>
>> -static int scrub_fs_info(int fd, char *path,
>> +static int scrub_fs_info( char *path,
>>  				struct btrfs_ioctl_fs_info_args *fi_args,
>>  				struct btrfs_ioctl_dev_info_args **di_ret)
>>  {
>>  	int ret = 0;
>>  	int ndevs = 0;
>>  	int i = 1;
>> +	int fd;
>>  	struct btrfs_fs_devices *fs_devices_mnt = NULL;
>>  	struct btrfs_ioctl_dev_info_args *di_args;
>>  	char mp[BTRFS_PATH_NAME_MAX + 1];
>>
>>  	memset(fi_args, 0, sizeof(*fi_args));
>>
>> +	fd  = open_file_or_dir(path);
>> +	if (fd < 0) {
>> +		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
>> +		return -1;
>> +	}
>> +
>>  	ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
>>  	if (ret && errno == EINVAL) {
>>  		/* path is no mounted btrfs. try if it's a device */
>> @@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
>>  		if (fd < 0)
>>  			return -errno;
>>  	} else if (ret) {
>> +		close(fd);
>>  		return -errno;
>>  	}
>>
>> -	if (!fi_args->num_devices)
>> +	if (!fi_args->num_devices){
>> +		close(fd);
>>  		return 0;
>> +	}
>>
>>  	di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
>> -	if (!di_args)
>> +	if (!di_args){
>> +		close(fd);
>>  		return -errno;
>> +	}
>>
>>  	for (; i <= fi_args->max_id; ++i) {
>>  		BUG_ON(ndevs >= fi_args->num_devices);
>>  		ret = scrub_device_info(fd, i, &di_args[ndevs]);
>>  		if (ret == -ENODEV)
>>  			continue;
>> -		if (ret)
>> +		if (ret){
>> +			close(fd);
>>  			return ret;
>> +		}
>>  		++ndevs;
>>  	}
>>
>>  	BUG_ON(ndevs == 0);
>>
>> +	close(fd);
>>  	return 0;
>>  }
>>
>> @@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int
>> resume)
>>  		return 12;
>>  	}
>>
>> -	ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
>> +	ret = scrub_fs_info(path, &fi_args, &di_args);
>>  	if (ret) {
>>  		ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
>>  		    "%s\n", strerror(-ret));
>> @@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
>>  		.sun_family = AF_UNIX,
>>  	};
>>  	int ret;
>> -	int fdmnt;
>>  	int i;
>>  	int print_raw = 0;
>>  	int do_stats_per_dev = 0;
>> @@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)
>>
>>  	path = argv[optind];
>>
>> -	fdmnt = open_file_or_dir(path);
>> -	if (fdmnt < 0) {
>> -		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
>> -		return 12;
>> -	}
>> -
>> -	ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
>> +	ret = scrub_fs_info(path, &fi_args, &di_args);
>>  	if (ret) {
>>  		fprintf(stderr, "ERROR: getting dev info for scrub failed: "
>>  				"%s\n", strerror(-ret));
>> @@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
>>  out:
>>  	free_history(past_scrubs);
>>  	free(di_args);
>> -	close(fdmnt);
>>  	if (fdres > -1)
>>  		close(fdres);
>>
>>
>>
>>
>>
>>
> 


[-- Attachment #2: fd-leaking.diff --]
[-- Type: text/x-patch, Size: 2707 bytes --]

diff --git a/cmds-scrub.c b/cmds-scrub.c
index c4503f4..6313e02 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
 	return ret ? -errno : 0;
 }
 
-static int scrub_fs_info(int fd, char *path,
+static int scrub_fs_info(char *path,
 				struct btrfs_ioctl_fs_info_args *fi_args,
 				struct btrfs_ioctl_dev_info_args **di_ret)
 {
 	int ret = 0;
 	int ndevs = 0;
 	int i = 1;
+	int fd;
 	struct btrfs_fs_devices *fs_devices_mnt = NULL;
 	struct btrfs_ioctl_dev_info_args *di_args;
 	char mp[BTRFS_PATH_NAME_MAX + 1];
 
 	memset(fi_args, 0, sizeof(*fi_args));
 
+	fd  = open_file_or_dir(path);
+	if (fd < 0) {
+		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
+		return -1;
+	}
+
 	ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
 	if (ret && errno == EINVAL) {
 		/* path is no mounted btrfs. try if it's a device */
@@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
 		if (fd < 0)
 			return -errno;
 	} else if (ret) {
+		close(fd);
 		return -errno;
 	}
 
-	if (!fi_args->num_devices)
+	if (!fi_args->num_devices) {
+		close(fd);
 		return 0;
+	}
 
 	di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
-	if (!di_args)
+	if (!di_args) {
+		close(fd);
 		return -errno;
+	}
 
 	for (; i <= fi_args->max_id; ++i) {
 		BUG_ON(ndevs >= fi_args->num_devices);
 		ret = scrub_device_info(fd, i, &di_args[ndevs]);
 		if (ret == -ENODEV)
 			continue;
-		if (ret)
+		if (ret) {
+			close(fd);
 			return ret;
+		}
 		++ndevs;
 	}
 
 	BUG_ON(ndevs == 0);
 
+	close(fd);
 	return 0;
 }
 
@@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int resume)
 		return 12;
 	}
 
-	ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
+	ret = scrub_fs_info(path, &fi_args, &di_args);
 	if (ret) {
 		ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
 		    "%s\n", strerror(-ret));
@@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
 		.sun_family = AF_UNIX,
 	};
 	int ret;
-	int fdmnt;
 	int i;
 	int print_raw = 0;
 	int do_stats_per_dev = 0;
@@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)
 
 	path = argv[optind];
 
-	fdmnt = open_file_or_dir(path);
-	if (fdmnt < 0) {
-		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
-		return 12;
-	}
-
-	ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
+	ret = scrub_fs_info(path, &fi_args, &di_args);
 	if (ret) {
 		fprintf(stderr, "ERROR: getting dev info for scrub failed: "
 				"%s\n", strerror(-ret));
@@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
 out:
 	free_history(past_scrubs);
 	free(di_args);
-	close(fdmnt);
 	if (fdres > -1)
 		close(fdres);