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 <mjguzik@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Sterling Jensen <sterlingjensen@users.noreply.github.com>
Closes #11098

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);
-
+	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, dataset, len) == 0)
-		return (dataset + len + (cwd[len-1] != '/'));
-
-	return (dataset);
+		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) {

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]

tests/zfs-tests/include/commands.cfg

@@ -184,6 +184,7 @@ export ZFS_FILES='zdb
     arc_summary
     arcstat
     dbufstat
+    mount.zfs
     zed
     zgenhostid
     zstream

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 \

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 '<dataset> <path>'"
+log_must $helper $fs $mntpoint
+log_must ismounted $fs
+force_unmount $fs
+
+log_note "Verify '\$PWD/<pool> <path>' prefix workaround"
+log_must $helper $PWD/$fs $mntpoint
+log_must ismounted $fs
+force_unmount $fs
+
+log_note "Verify '-f <dataset> <path>' fakemount"
+log_must $helper -f $fs $mntpoint
+log_mustnot ismounted $fs
+
+log_note "Verify '-o ro -v <dataset> <path>' verbose RO"
+log_must ${helper},ro -v $fs $mntpoint
+log_must ismounted $fs
+force_unmount $fs
+
+log_note "Verify '<device> <path>'"
+log_must $helper $vdev $mntpoint
+log_must ismounted $mntpoint
+log_must umount $TESTPOOL
+
+log_note "Verify '-o abc -s <device> <path>' 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"

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 <device> <path>' bad option fails"
+log_mustnot ${helper},abc $vdev $mntpoint
+
+log_note "Verify '\$NONEXISTFSNAME <path>' fails"
+log_mustnot $helper $NONEXISTFSNAME $mntpoint
+
+log_note "Verify '<dataset> (\$NONEXISTFSNAME|/dev/null)' fails"
+log_mustnot $helper $fs $NONEXISTFSNAME
+log_mustnot $helper $fs /dev/null
+
+log_note "Verify '/dev/null <path>' 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"