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 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?
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
