Fix unsafe string operations

Coverity caught unsafe use of `strcpy()` in `ztest_dmu_objset_own()`,
`nfs_init_tmpfile()` and `dump_snapshot()`. It also caught an unsafe use
of `strlcat()` in `nfs_init_tmpfile()`.

Inspired by this, I did an audit of every single usage of `strcpy()` and
`strcat()` in the code. If I could not prove that the usage was safe, I
changed the code to use either `strlcpy()` or `strlcat()`, depending on
which function was originally used. In some cases, `snprintf()` was used
to replace multiple uses of `strcat` because it was cleaner.

Whenever I changed a function, I preferred to use `sizeof(dst)` when the
compiler is able to provide the string size via that. When it could not
because the string was passed by a caller, I checked the entire call
tree of the function to find out how big the buffer was and hard coded
it. Hardcoding is less than ideal, but it is safe unless someone shrinks
the buffer sizes being passed.

Additionally, Coverity reported three more string related issues:

 * It caught a case where we do an overlapping memory copy in a call to
   `snprintf()`. We fix that via `kmem_strdup()` and `kmem_strfree()`.

 * It caught `sizeof (buf)` being used instead of `buflen` in
   `zdb_nicenum()`'s call to `zfs_nicenum()`, which is passed to
   `snprintf()`. We change that to pass `buflen`.

 * It caught a theoretical unterminated string passed to `strcmp()`.
   This one is likely a false positive, but we have the information
   needed to do this more safely, so we change this to silence the false
   positive not just in coverity, but potentially other static analysis
   tools too. We switch to `strncmp()`.

 * There was a false positive in tests/zfs-tests/cmd/dir_rd_update.c. We
   suppress it by switching to `snprintf()` since other static analysis
   tools might complain about it too. Interestingly, there is a possible
   real bug there too, since it assumes that the passed directory path
   ends with '/'. We add a '/' to fix that potential bug.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Closes #13913

cmd/zdb/zdb.c

@@ -985,7 +985,7 @@ zdb_nicenum(uint64_t num, char *buf, size_t buflen)
 	if (dump_opt['P'])
 		(void) snprintf(buf, buflen, "%llu", (longlong_t)num);
 	else
-		nicenum(num, buf, sizeof (buf));
+		nicenum(num, buf, buflen);
 }
 
 static const char histo_stars[] = "****************************************";

cmd/zfs/zfs_main.c

@@ -2459,7 +2459,8 @@ upgrade_set_callback(zfs_handle_t *zhp, void *data)
 			cb->cb_numupgraded++;
 		else
 			cb->cb_numfailed++;
-		(void) strcpy(cb->cb_lastfs, zfs_get_name(zhp));
+		(void) strlcpy(cb->cb_lastfs, zfs_get_name(zhp),
+		    sizeof (cb->cb_lastfs));
 	} else if (version > cb->cb_version) {
 		/* can't downgrade */
 		(void) printf(gettext("%s: can not be downgraded; "

cmd/zinject/translate.c

@@ -115,12 +115,12 @@ parse_pathname(const char *inpath, char *dataset, char *relpath,
 		return (-1);
 	}
 
-	(void) strcpy(dataset, mp.mnt_special);
+	(void) strlcpy(dataset, mp.mnt_special, MAXNAMELEN);
 
 	rel = fullpath + strlen(mp.mnt_mountp);
 	if (rel[0] == '/')
 		rel++;
-	(void) strcpy(relpath, rel);
+	(void) strlcpy(relpath, rel, MAXPATHLEN);
 
 	return (0);
 }
@@ -258,7 +258,7 @@ translate_record(err_type_t type, const char *object, const char *range,
 		}
 
 		dataset[0] = '\0';
-		(void) strcpy(poolname, object);
+		(void) strlcpy(poolname, object, MAXNAMELEN);
 		return (0);
 	}
 
@@ -298,7 +298,7 @@ translate_record(err_type_t type, const char *object, const char *range,
 	/*
 	 * Copy the pool name
 	 */
-	(void) strcpy(poolname, dataset);
+	(void) strlcpy(poolname, dataset, MAXNAMELEN);
 	if ((slash = strchr(poolname, '/')) != NULL)
 		*slash = '\0';
 

cmd/ztest.c

@@ -1544,7 +1544,7 @@ ztest_dmu_objset_own(const char *name, dmu_objset_type_t type,
 	char *cp = NULL;
 	char ddname[ZFS_MAX_DATASET_NAME_LEN];
 
-	strcpy(ddname, name);
+	strlcpy(ddname, name, sizeof (ddname));
 	cp = strchr(ddname, '@');
 	if (cp != NULL)
 		*cp = '\0';
@@ -3667,7 +3667,7 @@ ztest_vdev_attach_detach(ztest_ds_t *zd, uint64_t id)
 	oldguid = oldvd->vdev_guid;
 	oldsize = vdev_get_min_asize(oldvd);
 	oldvd_is_log = oldvd->vdev_top->vdev_islog;
-	(void) strcpy(oldpath, oldvd->vdev_path);
+	(void) strlcpy(oldpath, oldvd->vdev_path, MAXPATHLEN);
 	pvd = oldvd->vdev_parent;
 	pguid = pvd->vdev_guid;
 
@@ -3703,7 +3703,7 @@ ztest_vdev_attach_detach(ztest_ds_t *zd, uint64_t id)
 		if (newvd->vdev_ops == &vdev_draid_spare_ops)
 			newvd_is_dspare = B_TRUE;
 
-		(void) strcpy(newpath, newvd->vdev_path);
+		(void) strlcpy(newpath, newvd->vdev_path, MAXPATHLEN);
 	} else {
 		(void) snprintf(newpath, MAXPATHLEN, ztest_dev_template,
 		    ztest_opts.zo_dir, ztest_opts.zo_pool,
@@ -6145,8 +6145,8 @@ ztest_fault_inject(ztest_ds_t *zd, uint64_t id)
 		}
 		vd0 = sav->sav_vdevs[ztest_random(sav->sav_count)];
 		guid0 = vd0->vdev_guid;
-		(void) strcpy(path0, vd0->vdev_path);
-		(void) strcpy(pathrand, vd0->vdev_path);
+		(void) strlcpy(path0, vd0->vdev_path, MAXPATHLEN);
+		(void) strlcpy(pathrand, vd0->vdev_path, MAXPATHLEN);
 
 		leaf = 0;
 		leaves = 1;

contrib/pam_zfs_key/pam_zfs_key.c

@@ -558,9 +558,8 @@ zfs_key_config_get_dataset(zfs_key_config_t *config)
 		return (NULL);
 	}
 	ret[0] = 0;
-	strcat(ret, config->homes_prefix);
-	strcat(ret, "/");
-	strcat(ret, config->username);
+	(void) snprintf(ret, len + 1, "%s/%s", config->homes_prefix,
+	    config->username);
 	return (ret);
 }
 

lib/libshare/nfs.c

@@ -95,8 +95,8 @@ nfs_init_tmpfile(const char *prefix, const char *mdir, struct tmpfile *tmpf)
 		return (B_FALSE);
 	}
 
-	strcpy(tmpf->name, prefix);
-	strcat(tmpf->name, ".XXXXXXXX");
+	strlcpy(tmpf->name, prefix, sizeof (tmpf->name));
+	strlcat(tmpf->name, ".XXXXXXXX", sizeof (tmpf->name) - strlen(prefix));
 
 	int fd = mkostemp(tmpf->name, O_CLOEXEC);
 	if (fd == -1) {

lib/libspl/os/freebsd/mnttab.c

@@ -74,7 +74,7 @@ hasmntopt(struct mnttab *mnt, const char *opt)
 
 	if (mnt->mnt_mntopts == NULL)
 		return (NULL);
-	(void) strcpy(opts, mnt->mnt_mntopts);
+	(void) strlcpy(opts, mnt->mnt_mntopts, MNT_LINE_MAX);
 	f = mntopt(&opts);
 	for (; *f; f = mntopt(&opts)) {
 		if (strncmp(opt, f, strlen(opt)) == 0)

lib/libzfs/libzfs_sendrecv.c

@@ -1175,7 +1175,7 @@ dump_snapshot(zfs_handle_t *zhp, void *arg)
 			return (-1);
 	}
 
-	(void) strcpy(sdd->prevsnap, thissnap);
+	(void) strlcpy(sdd->prevsnap, thissnap, sizeof (sdd->prevsnap));
 	sdd->prevsnap_obj = zfs_prop_get_int(zhp, ZFS_PROP_OBJSETID);
 	zfs_close(zhp);
 	return (err);
@@ -1736,7 +1736,7 @@ zfs_send_resume_impl_cb_impl(libzfs_handle_t *hdl, sendflags_t *flags,
 	(void) nvlist_lookup_uint64(resume_nvl, "fromguid", &fromguid);
 
 	if (flags->saved) {
-		(void) strcpy(name, toname);
+		(void) strlcpy(name, toname, sizeof (name));
 	} else {
 		error = guid_to_name(hdl, toname, toguid, B_FALSE, name);
 		if (error != 0) {
@@ -2880,7 +2880,7 @@ recv_rename(libzfs_handle_t *hdl, const char *name, const char *tryname,
 		goto out;
 
 	if (tryname) {
-		(void) strcpy(newname, tryname);
+		(void) strlcpy(newname, tryname, ZFS_MAX_DATASET_NAME_LEN);
 		if (flags->verbose) {
 			(void) printf("attempting rename %s to %s\n",
 			    name, newname);
@@ -4331,7 +4331,7 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
 		err = nvlist_lookup_string(rcvprops,
 		    zfs_prop_to_name(ZFS_PROP_KEYLOCATION), &keylocation);
 		if (err == 0) {
-			strcpy(tmp_keylocation, keylocation);
+			strlcpy(tmp_keylocation, keylocation, MAXNAMELEN);
 			(void) nvlist_remove_all(rcvprops,
 			    zfs_prop_to_name(ZFS_PROP_KEYLOCATION));
 		}
@@ -4498,18 +4498,20 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
 		(void) snprintf(errbuf, sizeof (errbuf), dgettext(TEXT_DOMAIN,
 		    "cannot receive new filesystem stream"));
 
-		(void) strcpy(name, destsnap);
+		(void) strlcpy(name, destsnap, sizeof (name));
 		cp = strrchr(name, '/');
 		if (cp)
 			*cp = '\0';
 		if (cp &&
 		    !zfs_dataset_exists(hdl, name, ZFS_TYPE_DATASET)) {
 			char suffix[ZFS_MAX_DATASET_NAME_LEN];
-			(void) strcpy(suffix, strrchr(destsnap, '/'));
+			(void) strlcpy(suffix, strrchr(destsnap, '/'),
+			    sizeof (suffix));
 			if (guid_to_name(hdl, name, parent_snapguid,
 			    B_FALSE, destsnap) == 0) {
 				*strchr(destsnap, '@') = '\0';
-				(void) strcat(destsnap, suffix);
+				(void) strlcat(destsnap, suffix,
+				    sizeof (destsnap) - strlen(destsnap));
 			}
 		}
 	} else {
@@ -4527,7 +4529,7 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
 			    "cannot receive incremental stream"));
 		}
 
-		(void) strcpy(name, destsnap);
+		(void) strlcpy(name, destsnap, sizeof (name));
 		*strchr(name, '@') = '\0';
 
 		/*
@@ -4539,16 +4541,18 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
 		    strlen(sendfs)) != '\0' && *chopprefix != '@')) &&
 		    !zfs_dataset_exists(hdl, name, ZFS_TYPE_DATASET)) {
 			char snap[ZFS_MAX_DATASET_NAME_LEN];
-			(void) strcpy(snap, strchr(destsnap, '@'));
+			(void) strlcpy(snap, strchr(destsnap, '@'),
+			    sizeof (snap));
 			if (guid_to_name(hdl, name, drrb->drr_fromguid,
 			    B_FALSE, destsnap) == 0) {
 				*strchr(destsnap, '@') = '\0';
-				(void) strcat(destsnap, snap);
+				(void) strlcat(destsnap, snap,
+				    sizeof (destsnap) - strlen(destsnap));
 			}
 		}
 	}
 
-	(void) strcpy(name, destsnap);
+	(void) strlcpy(name, destsnap, sizeof (name));
 	*strchr(name, '@') = '\0';
 
 	redacted = DMU_GET_FEATUREFLAGS(drrb->drr_versioninfo) &
@@ -4930,7 +4934,7 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
 	if (err == 0 && snapprops_nvlist) {
 		zfs_cmd_t zc = {"\0"};
 
-		(void) strcpy(zc.zc_name, destsnap);
+		(void) strlcpy(zc.zc_name, destsnap, sizeof (zc.zc_name));
 		zc.zc_cookie = B_TRUE; /* received */
 		zcmd_write_src_nvlist(hdl, &zc, snapprops_nvlist);
 		(void) zfs_ioctl(hdl, ZFS_IOC_SET_PROP, &zc);

lib/libzfs/os/linux/libzfs_util_os.c

@@ -145,8 +145,8 @@ libzfs_load_module(void)
 		if (pfds[0].revents & POLLIN) {
 			verify(read(ino, ev, evsz) >
 			    sizeof (struct inotify_event));
-			if (strcmp(ev->name, &ZFS_DEV[sizeof (ZFS_DEVDIR)])
-			    == 0) {
+			if (strncmp(ev->name, &ZFS_DEV[sizeof (ZFS_DEVDIR)],
+			    ev->len) == 0) {
 				ret = 0;
 				break;
 			}

module/zfs/dmu_send.c

@@ -3016,7 +3016,8 @@ dmu_send_estimate_fast(dsl_dataset_t *origds, dsl_dataset_t *fromds,
 
 		dsl_dataset_name(origds, dsname);
 		(void) strcat(dsname, "/");
-		(void) strcat(dsname, recv_clone_name);
+		(void) strlcat(dsname, recv_clone_name,
+		    sizeof (dsname) - strlen(dsname));
 
 		err = dsl_dataset_hold(origds->ds_dir->dd_pool,
 		    dsname, FTAG, &ds);

module/zfs/dsl_dataset.c

@@ -2740,6 +2740,8 @@ dsl_get_mountpoint(dsl_dataset_t *ds, const char *dsname, char *value,
 		    relpath[0] != '\0'))
 			mnt = value + 1;
 
+		mnt = kmem_strdup(mnt);
+
 		if (relpath[0] == '\0') {
 			(void) snprintf(value, ZAP_MAXVALUELEN, "%s%s",
 			    root, mnt);
@@ -2749,6 +2751,7 @@ dsl_get_mountpoint(dsl_dataset_t *ds, const char *dsname, char *value,
 			    relpath);
 		}
 		kmem_free(buf, ZAP_MAXVALUELEN);
+		kmem_strfree(mnt);
 	}
 
 	return (0);

tests/zfs-tests/cmd/dir_rd_update.c

@@ -63,13 +63,12 @@ main(int argc, char **argv)
 	}
 
 	cp1 = argv[1];
-	if (strlen(cp1) >= (sizeof (dirpath) - strlen("TMP_DIR"))) {
+	if (strlen(cp1) >= (sizeof (dirpath) - strlen("/TMP_DIR"))) {
 		(void) printf("The string length of mount point is "
 		    "too large\n");
 		exit(-1);
 	}
-	(void) strcpy(&dirpath[0], (const char *)cp1);
-	(void) strcat(&dirpath[strlen(dirpath)], "TMP_DIR");
+	(void) snprintf(dirpath, sizeof (dirpath), "%s/TMP_DIR", cp1);
 
 	ret = mkdir(dirpath, 0777);
 	if (ret != 0) {

tests/zfs-tests/cmd/mmap_sync.c

@@ -73,8 +73,7 @@ main(int argc, char *argv[])
 	filepath[0] = '\0';
 	char *file = &filepath[0];
 
-	strcat(file, testdir);
-	strcat(file, "/msync_file");
+	(void) snprintf(file, 512, "%s/msync_file", testdir);
 
 	const int LEN = 8;
 	cleanup(file);