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 <kreij...@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);
>>
>>
>>
>>
>>
>>
>
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);
