Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libzfs zpool_find_vdev() overwrites pool config #4312

Closed
don-brady opened this issue Feb 5, 2016 · 6 comments
Closed

libzfs zpool_find_vdev() overwrites pool config #4312

don-brady opened this issue Feb 5, 2016 · 6 comments
Milestone

Comments

@don-brady
Copy link
Contributor

In zfsonlinx, the libzfs zpool_find_vdev() function has the side-effect of overwriting the pool config associated with the pool handle. Found while integration zfs_mod.c from illumos.

code path of overwrite:

  1. zpool_find_vdev() grabs config nvlist from pool handle and extracts vdev tree
  2. calls into vdev_to_nvlist_iter() with vdev tree, visiting each vdev
  3. calls into zfs_strcmp_pathname() with 'path' string value
  4. calls strtok() on above string and path value is overwritten

an easy way to reproduce is (see actual code below):

$ gcc -m64 -lzfs -lnvpair -I/usr/include/libzfs/ -I/usr/include/libspl zpool_find_vdev.c -o zpool_find_vdev
$ sudo ./zpool_find_vdev serenity

config vdev paths before:
vdev 01123202461778220001: /dev/disk/by-id/scsi-350000394a8caede4-part1
vdev 11514177655449603062: /dev/disk/by-id/scsi-350000394a8cb11f0-part1
vdev 01435883551868357921: /dev/disk/by-id/scsi-350000394a8cb4d8c-part1
vdev 07294385912884184264: /dev/disk/by-id/scsi-350000394a8ca4fc4-part1
vdev 03405823394303353173: /dev/disk/by-id/scsi-350000394a8cb3d64-part1
vdev 06846423427570562778: /dev/disk/by-id/scsi-350000394a8ca4fbc-part1
vdev 12727135398764565842: /dev/disk/by-id/scsi-350000394a8cb2d3c-part1
vdev 12336940145533931721: /dev/disk/by-id/scsi-350000394a8ca4fc0-part1

config vdev paths after:
vdev 01123202461778220001: /dev
vdev 11514177655449603062: /dev
vdev 01435883551868357921: /dev
vdev 07294385912884184264: /dev
vdev 03405823394303353173: /dev
vdev 06846423427570562778: /dev
vdev 12727135398764565842: /dev
vdev 12336940145533931721: /dev

stand-alone code that repoduces the problem:

#include <stdio.h>
#include <libzfs/libnvpair.h>
#include <libzfs/libzfs.h>


static void
print_vdev_paths(zpool_handle_t *zhp, nvlist_t *nvl)
{
    char *path;
    uint_t c, children;
    nvlist_t **child;
    uint64_t guid;

    if (nvlist_lookup_nvlist_array(nvl, ZPOOL_CONFIG_CHILDREN,
        &child, &children) == 0) {
        for (c = 0; c < children; c++)
            print_vdev_paths(zhp, child[c]);
        return;
    }
    (void) nvlist_lookup_uint64(nvl, ZPOOL_CONFIG_GUID, &guid);
    (void) nvlist_lookup_string(nvl, ZPOOL_CONFIG_PATH, &path);

    if (guid && path)
        (void) fprintf(stdout, "vdev %020llu: %s\n", guid, path);
}

int
main(int argc, char **argv)
{
    libzfs_handle_t *zfshdl;
    zpool_handle_t *zhp;

    zfshdl = libzfs_init();

    if ((zhp = zpool_open(zfshdl, argv[1])) != NULL) {
        nvlist_t *config, *tree;
        boolean_t tmp;

        config = zpool_get_config(zhp, NULL);
        (void) nvlist_lookup_nvlist(config, ZPOOL_CONFIG_VDEV_TREE, &tree);

        (void) fprintf(stdout, "\nconfig vdev paths before:\n");
        print_vdev_paths(zhp, tree);

        (void) zpool_find_vdev(zhp, "anything", &tmp, &tmp, &tmp);

        (void) fprintf(stdout, "\nconfig vdev paths after:\n");
        print_vdev_paths(zhp, tree);

        zpool_close(zhp);
    }

    libzfs_fini(zfshdl);
}
@behlendorf
Copy link
Contributor

That's not good. The problem is with zfs_strcmp_pathname() it was added as a wrapper function to replace the illumos specific path logic in vdev_to_nvlist_iter(). Unfortunately, it has side effects because it uses strtok() to parse the string.

The following tiny patch solves the issue but there may be a cleaner way to do this.

diff --git a/lib/libzfs/libzfs_util.c b/lib/libzfs/libzfs_util.c
index fa55fd3..65b04c5 100644
--- a/lib/libzfs/libzfs_util.c
+++ b/lib/libzfs/libzfs_util.c
@@ -1029,16 +1029,18 @@ zfs_strcmp_pathname(char *name, char *cmp, int wholedisk)
        int path_len, cmp_len;
        char path_name[MAXPATHLEN];
        char cmp_name[MAXPATHLEN];
-       char *dir;
+       char *dir, *dup;

        /* Strip redundant slashes if one exists due to ZPOOL_IMPORT_PATH */
        memset(cmp_name, 0, MAXPATHLEN);
-       dir = strtok(cmp, "/");
+       dup = strdup(cmp);
+       dir = strtok(dup, "/");
        while (dir) {
                strcat(cmp_name, "/");
                strcat(cmp_name, dir);
                dir = strtok(NULL, "/");
        }
+       free(dup);

        if (name[0] != '/')
                return (zfs_strcmp_shortname(name, cmp_name, wholedisk));
$ sudo ctest pool1

config vdev paths before:
vdev 17705542127730078299: /dev/vdc1

config vdev paths after:
vdev 17705542127730078299: /dev/vdc1

It occurs to me this might be causing other subtle problems in the utilities, but it's hard to say.

@don-brady
Copy link
Contributor Author

Thanks for the rapid response. For me the first vdev find operation was preventing future ones from succeeding.

@behlendorf
Copy link
Contributor

No problem, you did all the hard work for me! I've opened #4315 with the proposed fix above to get comment and so we don't loose track of it.

@dracwyrm
Copy link

dracwyrm commented Feb 6, 2016

I've searched and everywhere I look, people say not to use strtok() because it's not thread safe and can't be used in multi-threaded programs. Though there are not a lot of replacements out there. The only thing I've seen is this: http://www.fi.muni.cz/usr/jkucera/tic/tic0226.html which demonstrates how to write a safe replacement that doesn't overwrite the files. But, that's even more lines of code.

In other words, I don't think there really is a more elegant way and is the same method on all the help sites.

@don-brady
Copy link
Contributor Author

@ dracwyrm thanks for checking into strtok(). In general libzfs is not thread safe in that each instance (lib handle or in this case a pool handle) keeps unprotected global state. For example, the error state functions assume a single thread is making calls.

@behlendorf
Copy link
Contributor

@dracwyrm sorry I meant to comment on that. You raise a good point but the unfortunate reality is as @don-brady pointed out, the whole thing isn't thread-safe. There's definitely a case to be made for not making it any worse but let's tackle that in another patch. We can convert all the strtok(3) users to strtok_r(3) which is thread-safe.

nedbass pushed a commit that referenced this issue Mar 23, 2016
When extracting tokens from the string strtok(2) is allowed to modify
the passed buffer.  Therefore the zfs_strcmp_pathname() function must
make a copy of the passed string before passing it to strtok(3).

Signed-off-by: Brian Behlendorf <[email protected]>
Signed-off-by: Don Brady <[email protected]>
Closes #4312
@behlendorf behlendorf added this to the 0.6.5.6 milestone Mar 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants