From a4ae4998cbae3bb451642272738ab4d59b9714c4 Mon Sep 17 00:00:00 2001 From: sterlingjensen <5555776+sterlingjensen@users.noreply.github.com> Date: Tue, 10 Nov 2020 17:50:44 -0600 Subject: [PATCH] Fix memleak in cmd/mount_zfs.c Convert dynamic allocation to static buffer, simplify parse_dataset function return path. Add tests specific to the mount helper. Reviewed-by: Mateusz Guzik Reviewed-by: Brian Behlendorf Signed-off-by: Sterling Jensen Closes #11098 --- cmd/mount_zfs/mount_zfs.c | 68 +++++++---------- tests/runfiles/linux.run | 3 +- tests/zfs-tests/include/commands.cfg | 1 + .../functional/cli_root/zfs_mount/Makefile.am | 2 + .../cli_root/zfs_mount/zfs_mount_013_pos.ksh | 76 +++++++++++++++++++ .../cli_root/zfs_mount/zfs_mount_014_neg.ksh | 68 +++++++++++++++++ 6 files changed, 176 insertions(+), 42 deletions(-) create mode 100755 tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_013_pos.ksh create mode 100755 tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_014_neg.ksh diff --git a/cmd/mount_zfs/mount_zfs.c b/cmd/mount_zfs/mount_zfs.c index ed9f167cc..13935a9cc 100644 --- a/cmd/mount_zfs/mount_zfs.c +++ b/cmd/mount_zfs/mount_zfs.c @@ -47,46 +47,34 @@ libzfs_handle_t *g_zfs; * is expected to be of the form pool/dataset, however may also refer to * a block device if that device contains a valid zfs label. */ -static char * -parse_dataset(char *dataset) +static void +parse_dataset(const char *target, char **dataset) { - char cwd[PATH_MAX]; - struct stat64 statbuf; - int error; - int len; - /* * We expect a pool/dataset to be provided, however if we're * given a device which is a member of a zpool we attempt to * extract the pool name stored in the label. Given the pool * name we can mount the root dataset. */ - error = stat64(dataset, &statbuf); - if (error == 0) { - nvlist_t *config; - char *name; - int fd; + int fd = open(target, O_RDONLY); + if (fd >= 0) { + nvlist_t *config = NULL; + if (zpool_read_label(fd, &config, NULL) != 0) + config = NULL; + if (close(fd)) + perror("close"); - fd = open(dataset, O_RDONLY); - if (fd < 0) - goto out; - - error = zpool_read_label(fd, &config, NULL); - (void) close(fd); - if (error) - goto out; - - error = nvlist_lookup_string(config, - ZPOOL_CONFIG_POOL_NAME, &name); - if (error) { + if (config) { + char *name = NULL; + if (!nvlist_lookup_string(config, + ZPOOL_CONFIG_POOL_NAME, &name)) + (void) strlcpy(*dataset, name, PATH_MAX); nvlist_free(config); - } else { - dataset = strdup(name); - nvlist_free(config); - return (dataset); + if (name) + return; } } -out: + /* * If a file or directory in your current working directory is * named 'dataset' then mount(8) will prepend your current working @@ -94,16 +82,14 @@ out: * behavior so we simply check for it and strip the prepended * patch when it is added. */ - if (getcwd(cwd, PATH_MAX) == NULL) - return (dataset); - - len = strlen(cwd); - - /* Do not add one when cwd already ends in a trailing '/' */ - if (strncmp(cwd, dataset, len) == 0) - return (dataset + len + (cwd[len-1] != '/')); - - return (dataset); + char cwd[PATH_MAX]; + if (getcwd(cwd, PATH_MAX) != NULL) { + int len = strlen(cwd); + /* Do not add one when cwd already ends in a trailing '/' */ + if (strncmp(cwd, target, len) == 0) + target += len + (cwd[len-1] != '/'); + } + strlcpy(*dataset, target, PATH_MAX); } /* @@ -176,7 +162,7 @@ main(int argc, char **argv) char badopt[MNT_LINE_MAX] = { '\0' }; char mtabopt[MNT_LINE_MAX] = { '\0' }; char mntpoint[PATH_MAX]; - char *dataset; + char dataset[PATH_MAX], *pdataset = dataset; unsigned long mntflags = 0, zfsflags = 0, remount = 0; int sloppy = 0, fake = 0, verbose = 0, nomtab = 0, zfsutil = 0; int error, c; @@ -232,7 +218,7 @@ main(int argc, char **argv) return (MOUNT_USAGE); } - dataset = parse_dataset(argv[0]); + parse_dataset(argv[0], &pdataset); /* canonicalize the mount point */ if (realpath(argv[1], mntpoint) == NULL) { diff --git a/tests/runfiles/linux.run b/tests/runfiles/linux.run index ac4d6af1c..d8312afd3 100644 --- a/tests/runfiles/linux.run +++ b/tests/runfiles/linux.run @@ -47,7 +47,8 @@ tests = ['zfs_003_neg'] tags = ['functional', 'cli_root', 'zfs'] [tests/functional/cli_root/zfs_mount:Linux] -tests = ['zfs_mount_006_pos', 'zfs_mount_008_pos', 'zfs_multi_mount'] +tests = ['zfs_mount_006_pos', 'zfs_mount_008_pos', 'zfs_mount_013_pos', + 'zfs_mount_014_neg', 'zfs_multi_mount'] tags = ['functional', 'cli_root', 'zfs_mount'] [tests/functional/cli_root/zfs_share:Linux] diff --git a/tests/zfs-tests/include/commands.cfg b/tests/zfs-tests/include/commands.cfg index 21d1950e7..2b81e1c19 100644 --- a/tests/zfs-tests/include/commands.cfg +++ b/tests/zfs-tests/include/commands.cfg @@ -184,6 +184,7 @@ export ZFS_FILES='zdb arc_summary arcstat dbufstat + mount.zfs zed zgenhostid zstream diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/Makefile.am b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/Makefile.am index 37c094238..8c90b2e75 100644 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/Makefile.am +++ b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/Makefile.am @@ -14,6 +14,8 @@ dist_pkgdata_SCRIPTS = \ zfs_mount_010_neg.ksh \ zfs_mount_011_neg.ksh \ zfs_mount_012_pos.ksh \ + zfs_mount_013_pos.ksh \ + zfs_mount_014_neg.ksh \ zfs_mount_all_001_pos.ksh \ zfs_mount_all_fail.ksh \ zfs_mount_all_mountpoints.ksh \ diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_013_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_013_pos.ksh new file mode 100755 index 000000000..9a62ffb02 --- /dev/null +++ b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_013_pos.ksh @@ -0,0 +1,76 @@ +#!/bin/ksh -p +# +# CDDL HEADER START +# +# This file and its contents are supplied under the terms of the +# Common Development and Distribution License ("CDDL"), version 1.0. +# You may only use this file in accordance with the terms of version +# 1.0 of the CDDL. +# +# A full copy of the text of the CDDL should have accompanied this +# source. A copy of the CDDL is also available via the Internet at +# http://www.illumos.org/license/CDDL. +# +# CDDL HEADER END +# + +. $STF_SUITE/include/libtest.shlib +. $STF_SUITE/tests/functional/cli_root/zfs_mount/zfs_mount.kshlib + +# +# DESCRIPTION: +# Verify zfs mount helper functions for both devices and pools. +# + +verify_runnable "both" + +set -A vdevs $(get_disklist_fullpath $TESTPOOL) +vdev=${vdevs[0]} +mntpoint=$TESTDIR/$TESTPOOL +helper="mount.zfs -o zfsutil" +fs=$TESTPOOL/$TESTFS + +function cleanup +{ + log_must force_unmount $vdev + [[ -d $mntpoint ]] && log_must rm -rf $mntpoint + return 0 +} +log_onexit cleanup + +log_note "Verify zfs mount helper functions for both devices and pools" + +# Ensure that the ZFS filesystem is unmounted +force_unmount $fs +log_must mkdir -p $mntpoint + +log_note "Verify ' '" +log_must $helper $fs $mntpoint +log_must ismounted $fs +force_unmount $fs + +log_note "Verify '\$PWD/ ' prefix workaround" +log_must $helper $PWD/$fs $mntpoint +log_must ismounted $fs +force_unmount $fs + +log_note "Verify '-f ' fakemount" +log_must $helper -f $fs $mntpoint +log_mustnot ismounted $fs + +log_note "Verify '-o ro -v ' verbose RO" +log_must ${helper},ro -v $fs $mntpoint +log_must ismounted $fs +force_unmount $fs + +log_note "Verify ' '" +log_must $helper $vdev $mntpoint +log_must ismounted $mntpoint +log_must umount $TESTPOOL + +log_note "Verify '-o abc -s ' sloppy option" +log_must ${helper},abc -s $vdev $mntpoint +log_must ismounted $mntpoint +log_must umount $TESTPOOL + +log_pass "zfs mount helper correctly handles both device and pool strings" \ No newline at end of file diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_014_neg.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_014_neg.ksh new file mode 100755 index 000000000..5cf0bc7b3 --- /dev/null +++ b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_014_neg.ksh @@ -0,0 +1,68 @@ +#!/bin/ksh -p +# +# CDDL HEADER START +# +# This file and its contents are supplied under the terms of the +# Common Development and Distribution License ("CDDL"), version 1.0. +# You may only use this file in accordance with the terms of version +# 1.0 of the CDDL. +# +# A full copy of the text of the CDDL should have accompanied this +# source. A copy of the CDDL is also available via the Internet at +# http://www.illumos.org/license/CDDL. +# +# CDDL HEADER END +# + +. $STF_SUITE/include/libtest.shlib +. $STF_SUITE/tests/functional/cli_root/zfs_mount/zfs_mount.kshlib + +# +# DESCRIPTION: +# Verify zfs mount helper failure on known bad parameters +# + +verify_runnable "both" + +set -A vdevs $(get_disklist_fullpath $TESTPOOL) +vdev=${vdevs[0]} + +mntpoint="$(get_prop mountpoint $TESTPOOL)" +helper="mount.zfs -o zfsutil" +fs=$TESTPOOL/$TESTFS + +function cleanup +{ + log_must force_unmount $vdev + return 0 +} +log_onexit cleanup + +log_note "Verify zfs mount helper failure on known bad parameters" + +# Ensure that the ZFS filesystem is unmounted. +force_unmount $fs + +log_note "Verify failure without '-o zfsutil'" +log_mustnot mount.zfs $fs $mntpoint + +log_note "Verify '-o abc ' bad option fails" +log_mustnot ${helper},abc $vdev $mntpoint + +log_note "Verify '\$NONEXISTFSNAME ' fails" +log_mustnot $helper $NONEXISTFSNAME $mntpoint + +log_note "Verify ' (\$NONEXISTFSNAME|/dev/null)' fails" +log_mustnot $helper $fs $NONEXISTFSNAME +log_mustnot $helper $fs /dev/null + +log_note "Verify '/dev/null ' fails" +log_mustnot $helper /dev/null $mntpoint + +log_note "Verify '[device|pool]' fails" +log_mustnot mount.zfs +log_mustnot $helper +log_mustnot $helper $vdev +log_mustnot $helper $TESTPOOL + +log_pass "zfs mount helper fails when expected" \ No newline at end of file