Goffredo Baroncelli
2012-Apr-24 18:43 UTC
[btrfs-progs] [bug][patch] Leaking file handle in scrub_fs_info()
Hi Jan,
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);
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Jan Schmidt
2012-Apr-25 09:39 UTC
Re: [btrfs-progs] [bug][patch] Leaking file handle in scrub_fs_info()
Hi Goffredo, On 24.04.2012 20:43, 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.It''s only a single fd that''s closed upon exit, but anyway...> 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 returnedYou''re right, that''s unintended. I admit that I haven''t tested passing a device instead of a mountpoint that much.> 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 ?My naive approach would be to pass an int * to scrub_fs_info. One has to make sure that scrub_start doesn''t rely on "fdmnt" not being updated. After skipping through it, I think it expects "fdmnt" to be an open fd, and it looks like it should be exactly the one used in scrub_fs_info. Would you like to test the int * approach?> 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,^ Apart from my proposed solution, the spacing in your patch doesn''t follow the style guide. No space here.> 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){^ space here> + close(fd); > return 0; > + } > > di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args)); > - if (!di_args) > + if (!di_args){^ space here> + 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){^ space here> + 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); > > > > > >Thanks! -Jan -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Goffredo Baroncelli
2012-Apr-25 19:07 UTC
Re: [btrfs-progs] [bug][patch] Leaking file handle in scrub_fs_info()
On 04/25/2012 11:39 AM, Jan Schmidt wrote:> Hi Goffredo, > > On 24.04.2012 20:43, 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. > > It''s only a single fd that''s closed upon exit, but anyway...Ironically, quite often the problem doesn''t exist at all, because if I 1) open a file descriptr 2) close the file descriptr 3) open a new file descriptor For 1) and 2) the kernel re-use the same file descriptor. However I think that the scrub_fs_info() could be a more generic function (not only scrub related) which could be reused in more places. So a more polite behaviour it is needed.> >> 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 > > You''re right, that''s unintended. I admit that I haven''t tested passing a > device instead of a mountpoint that much. > >> 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 ? > > My naive approach would be to pass an int * to scrub_fs_info. One has to > make sure that scrub_start doesn''t rely on "fdmnt" not being updated. > After skipping through it, I think it expects "fdmnt" to be an open fd, > and it looks like it should be exactly the one used in scrub_fs_info. > Would you like to test the int * approach?Frankly speaking, I don''t like the idea that scrub_fs_info() could change the file descriptor. I think that it is not good design. I would like to suggest the following options: 1) scrub_fs_info() opens a file descriptor on the basis of the path parameters. And then it returns this file descriptor (or via return or via a int * parameters) or 2) we move the re-open logic outside the function itself. So scrub_start() don''t need to change the file descriptor. I prefer the options #2, also because this is a quite common problem: we can create a generic function which returns a good file descriptor when a filesystem path and/or the device is passed What do you think ? Finally I will take in account your suggestion regarding the spacing. BR G.Baroncelli> >> 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, > ^ > Apart from my proposed solution, the spacing in your patch doesn''t > follow the style guide. No space here. > >> 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){ > ^ > space here > >> + close(fd); >> return 0; >> + } >> >> di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args)); >> - if (!di_args) >> + if (!di_args){ > ^ > space here > >> + 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){ > ^ > space here > >> + 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); >> >> >> >> >> >> > > Thanks! > -Jan > . >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hugo Mills
2012-Jun-05 11:01 UTC
Re: [btrfs-progs] [bug][patch] Leaking file handle in scrub_fs_info()
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); > > > > > >-- === Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk == PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk --- Computer Science is not about computers, any more than --- astronomy is about telescopes.
Goffredo Baroncelli
2012-Jun-05 17:26 UTC
Re: [btrfs-progs] [bug][patch V2] Leaking file handle in scrub_fs_info()
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);
>>
>>
>>
>>
>>
>>
>
Hugo Mills
2012-Jun-05 18:19 UTC
Re: [btrfs-progs] [bug][patch V2] Leaking file handle in scrub_fs_info()
On Tue, Jun 05, 2012 at 07:26:34PM +0200, Goffredo Baroncelli wrote:> Hi Hugo, > > I was not able to reproduce your error with my repository: I pulled it > without problem.Well, I''d have normally written it off as a corrupt local repo at this end -- except that David Sterba said he''d also seen a similar problem pulling from your repo.> 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.Not really. Your mailer is breaking things (there''s a wrapped line in there, and it''s heavily whitespace-damaged). Could you *please* learn to use git send-email? Every single patch of yours that I''ve seen on the mailing list is badly broken, and takes an age to get it to the point where "git am" will look at it cleanly. It''s incredibly frustrating. This time around, I think I''ve managed to get it to work OK, but it''s taken me 10 minutes of buggering around, and I''m getting irritable, which isn''t a good thing when I''m meant to be having a quiet relaxing week off work... Hugo.> -------------------- > > 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); > >> > >> > >> > >> > >> > >> > > >> 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); >-- === Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk == PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk --- Great oxymorons of the world, no. 5: Manifesto Promise ---
Goffredo Baroncelli
2012-Jun-05 20:12 UTC
Re: [btrfs-progs] [bug][patch V2] Leaking file handle in scrub_fs_info()
Hi Hugo, On 06/05/2012 08:19 PM, Hugo Mills wrote:> On Tue, Jun 05, 2012 at 07:26:34PM +0200, Goffredo Baroncelli > wrote: >> Hi Hugo, >> >> I was not able to reproduce your error with my repository: I >> pulled it without problem. > > Well, I''d have normally written it off as a corrupt local repo at > this end -- except that David Sterba said he''d also seen a similar > problem pulling from your repo. > >> 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. > > Not really. Your mailer is breaking things (there''s a wrapped line > in there, and it''s heavily whitespace-damaged). Could you *please* > learn to use git send-email? Every single patch of yours that I''ve > seen on the mailing list is badly broken, and takes an age to get > it to the point where "git am" will look at it cleanly. It''s > incredibly frustrating. This time around, I think I''ve managed to > get it to work OK, but it''s taken me 10 minutes of buggering > around, and I''m getting irritable, which isn''t a good thing when > I''m meant to be having a quiet relaxing week off work...I perfectly understand what means loosing 10 minutes instead of 1 seconds :-) But was you able to use the patch "attached" (not the one inlined ) ?> > Hugo. > >> -------------------- >> >> 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); >>>> >>>> >>>> >>>> >>>> >>>> >>> >> > >> 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); >> > >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html