Cleanup: Switch to strlcpy from strncpy

Coverity found a bug in `zfs_secpolicy_create_clone()` where it is
possible for us to pass an unterminated string when `zfs_get_parent()`
returns an error. Upon inspection, it is clear that using `strlcpy()`
would have avoided this issue.

Looking at the codebase, there are a number of other uses of `strncpy()`
that are unsafe and even when it is used safely, switching to
`strlcpy()` would make the code more readable. Therefore, we switch all
instances where we use `strncpy()` to use `strlcpy()`.

Unfortunately, we do not portably have access to `strlcpy()` in
tests/zfs-tests/cmd/zfs_diff-socket.c because it does not link to
libspl. Modifying the appropriate Makefile.am to try to link to it
resulted in an error from the naming choice used in the file. Trying to
disable the check on the file did not work on FreeBSD because Clang
ignores `#undef` when a definition is provided by `-Dstrncpy(...)=...`.
We workaround that by explictly including the C file from libspl into
the test. This makes things build correctly everywhere.

We add a deprecation warning to `config/Rules.am` and suppress it on the
remaining `strncpy()` usage. `strlcpy()` is not portably avaliable in
tests/zfs-tests/cmd/zfs_diff-socket.c, so we use `snprintf()` there as a
substitute.

This patch does not tackle the related problem of `strcpy()`, which is
even less safe. Thankfully, a quick inspection found that it is used far
more correctly than strncpy() was used. A quick inspection did not find
any problems with `strcpy()` usage outside of zhack, but it should be
said that I only checked around 90% of them.

Lastly, some of the fields in kstat_t varied in size by 1 depending on
whether they were in userspace or in the kernel. The origin of this
discrepancy appears to be 04a479f706 where
it was made for no apparent reason. It conflicts with the comment on
KSTAT_STRLEN, so we shrink the kernel field sizes to match the userspace
field sizes.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Closes #13876

cmd/zfs/zfs_main.c

@@ -6357,8 +6357,8 @@ zfs_do_hold_rele_impl(int argc, char **argv, boolean_t holding)
 			++errors;
 			continue;
 		}
-		(void) strncpy(parent, path, delim - path);
+		(void) strlcpy(parent, path, MIN(sizeof (parent),
-		parent[delim - path] = '\0';
+		    delim - path + 1));
 
 		zhp = zfs_open(g_zfs, parent,
 		    ZFS_TYPE_FILESYSTEM | ZFS_TYPE_VOLUME);

cmd/ztest.c

@@ -1137,7 +1137,8 @@ process_options(int argc, char **argv)
 			goto invalid;
 
 		int dirlen = strrchr(val, '/') - val;
-		strncpy(zo->zo_alt_libpath, val, dirlen);
+		strlcpy(zo->zo_alt_libpath, val,
+		    MIN(sizeof (zo->zo_alt_libpath), dirlen + 1));
 		invalid_what = "library path", val = zo->zo_alt_libpath;
 		if (strrchr(val, '/') == NULL && (errno = EINVAL))
 			goto invalid;

config/Rules.am

@@ -52,6 +52,8 @@ AM_CPPFLAGS_NOCHECK += -D"asctime(...)=__attribute__((deprecated(\"Use strftime(
 AM_CPPFLAGS_NOCHECK += -D"asctime_r(...)=__attribute__((deprecated(\"Use strftime(3) instead!\"))) asctime_r(__VA_ARGS__)"
 AM_CPPFLAGS_NOCHECK += -D"gmtime(...)=__attribute__((deprecated(\"gmtime(3) isn't thread-safe. Use gmtime_r(3) instead!\"))) gmtime(__VA_ARGS__)"
 AM_CPPFLAGS_NOCHECK += -D"localtime(...)=__attribute__((deprecated(\"localtime(3) isn't thread-safe. Use localtime_r(3) instead!\"))) localtime(__VA_ARGS__)"
+AM_CPPFLAGS_NOCHECK += -D"strncpy(...)=__attribute__((deprecated(\"strncpy(3) is deprecated. Use strlcpy(3) instead!\"))) strncpy(__VA_ARGS__)"
+
 AM_CPPFLAGS += $(AM_CPPFLAGS_NOCHECK)
 
 if ASAN_ENABLED

include/os/freebsd/spl/sys/kstat.h

@@ -94,7 +94,7 @@ void seq_printf(struct seq_file *m, const char *fmt, ...);
 
 
 typedef struct kstat_module {
-	char ksm_name[KSTAT_STRLEN+1];		/* module name */
+	char ksm_name[KSTAT_STRLEN];		/* module name */
 	struct list_head ksm_module_list;	/* module linkage */
 	struct list_head ksm_kstat_list;	/* list of kstat entries */
 	struct proc_dir_entry *ksm_proc;	/* proc entry */
@@ -112,10 +112,10 @@ struct kstat_s {
 	kid_t		ks_kid;			/* unique kstat ID */
 	hrtime_t	ks_crtime;		/* creation time */
 	hrtime_t	ks_snaptime;		/* last access time */
-	char		ks_module[KSTAT_STRLEN+1]; /* provider module name */
+	char		ks_module[KSTAT_STRLEN]; /* provider module name */
 	int		ks_instance;		/* provider module instance */
-	char		ks_name[KSTAT_STRLEN+1]; /* kstat name */
+	char		ks_name[KSTAT_STRLEN]; /* kstat name */
-	char		ks_class[KSTAT_STRLEN+1]; /* kstat class */
+	char		ks_class[KSTAT_STRLEN]; /* kstat class */
 	uchar_t		ks_type;		/* kstat data type */
 	uchar_t		ks_flags;		/* kstat flags */
 	void		*ks_data;		/* kstat type-specific data */
@@ -181,7 +181,7 @@ typedef struct kstat_io {
 } kstat_io_t;
 
 typedef struct kstat_timer {
-	char		name[KSTAT_STRLEN+1]; /* event name */
+	char		name[KSTAT_STRLEN]; /* event name */
 	u_longlong_t	num_events;	 /* number of events */
 	hrtime_t	elapsed_time;	 /* cumulative elapsed time */
 	hrtime_t	min_time;	 /* shortest event duration */

include/os/linux/spl/sys/kstat.h

@@ -85,7 +85,7 @@ typedef int kid_t;				/* unique kstat id */
 typedef int kstat_update_t(struct kstat_s *, int); /* dynamic update cb */
 
 typedef struct kstat_module {
-	char ksm_name[KSTAT_STRLEN+1];		/* module name */
+	char ksm_name[KSTAT_STRLEN];		/* module name */
 	struct list_head ksm_module_list;	/* module linkage */
 	struct list_head ksm_kstat_list;	/* list of kstat entries */
 	struct proc_dir_entry *ksm_proc;	/* proc entry */
@@ -98,8 +98,8 @@ typedef struct kstat_raw_ops {
 } kstat_raw_ops_t;
 
 typedef struct kstat_proc_entry {
-	char	kpe_name[KSTAT_STRLEN+1];	/* kstat name */
+	char	kpe_name[KSTAT_STRLEN];		/* kstat name */
-	char	kpe_module[KSTAT_STRLEN+1];	/* provider module name */
+	char	kpe_module[KSTAT_STRLEN];	/* provider module name */
 	kstat_module_t		*kpe_owner;	/* kstat module linkage */
 	struct list_head	kpe_list;	/* kstat linkage */
 	struct proc_dir_entry	*kpe_proc;	/* procfs entry */
@@ -111,7 +111,7 @@ struct kstat_s {
 	hrtime_t	ks_crtime;		/* creation time */
 	hrtime_t	ks_snaptime;		/* last access time */
 	int		ks_instance;		/* provider module instance */
-	char		ks_class[KSTAT_STRLEN+1]; /* kstat class */
+	char		ks_class[KSTAT_STRLEN]; /* kstat class */
 	uchar_t		ks_type;		/* kstat data type */
 	uchar_t		ks_flags;		/* kstat flags */
 	void		*ks_data;		/* kstat type-specific data */
@@ -177,7 +177,7 @@ typedef struct kstat_io {
 } kstat_io_t;
 
 typedef struct kstat_timer {
-	char		name[KSTAT_STRLEN+1]; /* event name */
+	char		name[KSTAT_STRLEN]; /* event name */
 	u_longlong_t	num_events;	 /* number of events */
 	hrtime_t	elapsed_time;	 /* cumulative elapsed time */
 	hrtime_t	min_time;	 /* shortest event duration */

lib/libzfs/libzfs_dataset.c

@@ -717,8 +717,8 @@ zfs_open(libzfs_handle_t *hdl, const char *path, int types)
 		 * to get the parent dataset name only.
 		 */
 		assert(bookp - path < sizeof (dsname));
-		(void) strncpy(dsname, path, bookp - path);
+		(void) strlcpy(dsname, path,
-		dsname[bookp - path] = '\0';
+		    MIN(sizeof (dsname), bookp - path + 1));
 
 		/*
 		 * Create handle for the parent dataset.
@@ -3454,8 +3454,8 @@ check_parents(libzfs_handle_t *hdl, const char *path, uint64_t *zoned,
 	/* check to see if the pool exists */
 	if ((slash = strchr(parent, '/')) == NULL)
 		slash = parent + strlen(parent);
-	(void) strncpy(zc.zc_name, parent, slash - parent);
+	(void) strlcpy(zc.zc_name, parent,
-	zc.zc_name[slash - parent] = '\0';
+	    MIN(sizeof (zc.zc_name), slash - parent + 1));
 	if (zfs_ioctl(hdl, ZFS_IOC_OBJSET_STATS, &zc) != 0 &&
 	    errno == ENOENT) {
 		zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,

lib/libzfs/libzfs_diff.c

@@ -572,8 +572,7 @@ get_snapshot_names(differ_info_t *di, const char *fromsnap,
 		zfs_handle_t *zhp;
 
 		di->ds = zfs_alloc(di->zhp->zfs_hdl, tdslen + 1);
-		(void) strncpy(di->ds, tosnap, tdslen);
+		(void) strlcpy(di->ds, tosnap, tdslen + 1);
-		di->ds[tdslen] = '\0';
 
 		zhp = zfs_open(hdl, di->ds, ZFS_TYPE_FILESYSTEM);
 		while (zhp != NULL) {
@@ -609,8 +608,7 @@ get_snapshot_names(differ_info_t *di, const char *fromsnap,
 		int dslen = fdslen ? fdslen : tdslen;
 
 		di->ds = zfs_alloc(hdl, dslen + 1);
-		(void) strncpy(di->ds, fdslen ? fromsnap : tosnap, dslen);
+		(void) strlcpy(di->ds, fdslen ? fromsnap : tosnap, dslen + 1);
-		di->ds[dslen] = '\0';
 
 		di->fromsnap = zfs_asprintf(hdl, "%s%s", di->ds, atptrf);
 		if (tsnlen) {

lib/libzpool/taskq.c

@@ -276,7 +276,7 @@ taskq_create(const char *name, int nthreads, pri_t pri,
 	cv_init(&tq->tq_dispatch_cv, NULL, CV_DEFAULT, NULL);
 	cv_init(&tq->tq_wait_cv, NULL, CV_DEFAULT, NULL);
 	cv_init(&tq->tq_maxalloc_cv, NULL, CV_DEFAULT, NULL);
-	(void) strncpy(tq->tq_name, name, TASKQ_NAMELEN);
+	(void) strlcpy(tq->tq_name, name, sizeof (tq->tq_name));
 	tq->tq_flags = flags | TASKQ_ACTIVE;
 	tq->tq_active = nthreads;
 	tq->tq_nthreads = nthreads;

module/os/freebsd/spl/callb.c

@@ -160,8 +160,7 @@ callb_add_common(boolean_t (*func)(void *arg, int code),
 		    "too long -- truncated to %d chars",
 		    name, CB_MAXNAME);
 #endif
-	(void) strncpy(cp->c_name, name, CB_MAXNAME);
+	(void) strlcpy(cp->c_name, name, sizeof (cp->c_name));
-	cp->c_name[CB_MAXNAME] = '\0';
 
 	/*
 	 * Insert the new callb at the head of its class list.

module/os/freebsd/zfs/zfs_vfsops.c

@@ -1272,8 +1272,7 @@ getpoolname(const char *osname, char *poolname)
 	} else {
 		if (p - osname >= MAXNAMELEN)
 			return (ENAMETOOLONG);
-		(void) strncpy(poolname, osname, p - osname);
+		(void) strlcpy(poolname, osname, p - osname + 1);
-		poolname[p - osname] = '\0';
 	}
 	return (0);
 }

module/os/linux/spl/spl-kmem-cache.c

@@ -706,7 +706,7 @@ spl_kmem_cache_create(const char *name, size_t size, size_t align,
 		kfree(skc);
 		return (NULL);
 	}
-	strncpy(skc->skc_name, name, skc->skc_name_size);
+	strlcpy(skc->skc_name, name, skc->skc_name_size);
 
 	skc->skc_ctor = ctor;
 	skc->skc_dtor = dtor;

module/os/linux/spl/spl-kstat.c

@@ -390,7 +390,7 @@ kstat_create_module(char *name)
 
 	module = kmem_alloc(sizeof (kstat_module_t), KM_SLEEP);
 	module->ksm_proc = pde;
-	strlcpy(module->ksm_name, name, KSTAT_STRLEN+1);
+	strlcpy(module->ksm_name, name, KSTAT_STRLEN);
 	INIT_LIST_HEAD(&module->ksm_kstat_list);
 	list_add_tail(&module->ksm_module_list, &kstat_module_list);
 
@@ -479,8 +479,8 @@ kstat_proc_entry_init(kstat_proc_entry_t *kpep, const char *module,
 	kpep->kpe_owner = NULL;
 	kpep->kpe_proc = NULL;
 	INIT_LIST_HEAD(&kpep->kpe_list);
-	strncpy(kpep->kpe_module, module, KSTAT_STRLEN);
+	strlcpy(kpep->kpe_module, module, sizeof (kpep->kpe_module));
-	strncpy(kpep->kpe_name, name, KSTAT_STRLEN);
+	strlcpy(kpep->kpe_name, name, sizeof (kpep->kpe_name));
 }
 EXPORT_SYMBOL(kstat_proc_entry_init);
 
@@ -514,7 +514,7 @@ __kstat_create(const char *ks_module, int ks_instance, const char *ks_name,
 	ksp->ks_crtime = gethrtime();
 	ksp->ks_snaptime = ksp->ks_crtime;
 	ksp->ks_instance = ks_instance;
-	strncpy(ksp->ks_class, ks_class, KSTAT_STRLEN);
+	strlcpy(ksp->ks_class, ks_class, sizeof (ksp->ks_class));
 	ksp->ks_type = ks_type;
 	ksp->ks_flags = ks_flags;
 	ksp->ks_update = kstat_default_update;

module/os/linux/spl/spl-thread.c

@@ -92,7 +92,7 @@ __thread_create(caddr_t stk, size_t  stksize, thread_func_t func,
 		return (NULL);
 	}
 
-	strncpy(tp->tp_name, name, tp->tp_name_size);
+	strlcpy(tp->tp_name, name, tp->tp_name_size);
 
 	/*
 	 * Strip trailing "_thread" from passed name which will be the func

module/os/linux/spl/spl-zone.c

@@ -203,8 +203,7 @@ zone_dataset_attach(cred_t *cred, const char *dataset, int userns_fd)
 
 	zd = kmem_alloc(sizeof (zone_dataset_t) + dsnamelen + 1, KM_SLEEP);
 	zd->zd_dsnamelen = dsnamelen;
-	strncpy(zd->zd_dsname, dataset, dsnamelen);
+	strlcpy(zd->zd_dsname, dataset, dsnamelen + 1);
-	zd->zd_dsname[dsnamelen] = '\0';
 	INIT_LIST_HEAD(&zd->zd_list);
 	list_add_tail(&zd->zd_list, &zds->zds_datasets);
 

module/zfs/dsl_dir.c

@@ -428,8 +428,7 @@ getcomponent(const char *path, char *component, const char **nextp)
 	} else if (p[0] == '/') {
 		if (p - path >= ZFS_MAX_DATASET_NAME_LEN)
 			return (SET_ERROR(ENAMETOOLONG));
-		(void) strncpy(component, path, p - path);
+		(void) strlcpy(component, path, p - path + 1);
-		component[p - path] = '\0';
 		p++;
 	} else if (p[0] == '@') {
 		/*
@@ -440,8 +439,7 @@ getcomponent(const char *path, char *component, const char **nextp)
 			return (SET_ERROR(EINVAL));
 		if (p - path >= ZFS_MAX_DATASET_NAME_LEN)
 			return (SET_ERROR(ENAMETOOLONG));
-		(void) strncpy(component, path, p - path);
+		(void) strlcpy(component, path, p - path + 1);
-		component[p - path] = '\0';
 	} else {
 		panic("invalid p=%p", (void *)p);
 	}

module/zfs/dsl_prop.c

@@ -57,7 +57,7 @@ dodefault(zfs_prop_t prop, int intsz, int numints, void *buf)
 	if (zfs_prop_get_type(prop) == PROP_TYPE_STRING) {
 		if (intsz != 1)
 			return (SET_ERROR(EOVERFLOW));
-		(void) strncpy(buf, zfs_prop_default_string(prop),
+		(void) strlcpy(buf, zfs_prop_default_string(prop),
 		    numints);
 	} else {
 		if (intsz != 8 || numints < 1)
@@ -1019,8 +1019,8 @@ dsl_prop_get_all_impl(objset_t *mos, uint64_t propobj,
 			if (flags & DSL_PROP_GET_LOCAL)
 				continue;
 
-			(void) strncpy(buf, za.za_name, (suffix - za.za_name));
+			(void) strlcpy(buf, za.za_name,
-			buf[suffix - za.za_name] = '\0';
+			    MIN(sizeof (buf), suffix - za.za_name + 1));
 			propname = buf;
 
 			if (!(flags & DSL_PROP_GET_RECEIVED)) {

module/zfs/spa_misc.c

@@ -1654,7 +1654,7 @@ spa_altroot(spa_t *spa, char *buf, size_t buflen)
 	if (spa->spa_root == NULL)
 		buf[0] = '\0';
 	else
-		(void) strncpy(buf, spa->spa_root, buflen);
+		(void) strlcpy(buf, spa->spa_root, buflen);
 }
 
 int

module/zfs/zcp_get.c

@@ -608,8 +608,7 @@ parse_userquota_prop(const char *prop_name, zfs_userquota_prop_t *type,
 		 */
 		int domain_len = strrchr(cp, '-') - cp;
 		domain_val = kmem_alloc(domain_len + 1, KM_SLEEP);
-		(void) strncpy(domain_val, cp, domain_len);
+		(void) strlcpy(domain_val, cp, domain_len + 1);
-		domain_val[domain_len] = '\0';
 		cp += domain_len + 1;
 
 		(void) ddi_strtoll(cp, &end, 10, (longlong_t *)rid);

module/zfs/zfs_ioctl.c

@@ -741,7 +741,7 @@ zfs_get_parent(const char *datasetname, char *parent, int parentsize)
 	/*
 	 * Remove the @bla or /bla from the end of the name to get the parent.
 	 */
-	(void) strncpy(parent, datasetname, parentsize);
+	(void) strlcpy(parent, datasetname, parentsize);
 	cp = strrchr(parent, '@');
 	if (cp != NULL) {
 		cp[0] = '\0';

module/zfs/zio_inject.c

@@ -887,7 +887,7 @@ zio_inject_list_next(int *id, char *name, size_t buflen,
 	if (handler) {
 		*record = handler->zi_record;
 		*id = handler->zi_id;
-		(void) strncpy(name, spa_name(handler->zi_spa), buflen);
+		(void) strlcpy(name, spa_name(handler->zi_spa), buflen);
 		ret = 0;
 	} else {
 		ret = SET_ERROR(ENOENT);

tests/zfs-tests/cmd/draid.c

@@ -828,7 +828,7 @@ draid_generate(int argc, char *argv[])
 	}
 
 	if (argc > optind)
-		strncpy(filename, argv[optind], MAXPATHLEN - 1);
+		strlcpy(filename, argv[optind], sizeof (filename));
 	else {
 		(void) fprintf(stderr, "A FILE must be specified.\n");
 		return (1);
@@ -960,9 +960,9 @@ draid_verify(int argc, char *argv[])
 			return (ENOMEM);
 
 		if (realpath(argv[optind], abspath) != NULL)
-			strncpy(filename, abspath, MAXPATHLEN - 1);
+			strlcpy(filename, abspath, sizeof (filename));
 		else
-			strncpy(filename, argv[optind], MAXPATHLEN - 1);
+			strlcpy(filename, argv[optind], sizeof (filename));
 
 		free(abspath);
 	} else {
@@ -1169,7 +1169,7 @@ draid_dump(int argc, char *argv[])
 	}
 
 	if (argc > optind)
-		strncpy(filename, argv[optind], MAXPATHLEN - 1);
+		strlcpy(filename, argv[optind], sizeof (filename));
 	else {
 		(void) fprintf(stderr, "A FILE must be specified.\n");
 		return (1);
@@ -1206,7 +1206,7 @@ draid_table(int argc, char *argv[])
 	int error;
 
 	if (argc > optind)
-		strncpy(filename, argv[optind], MAXPATHLEN - 1);
+		strlcpy(filename, argv[optind], sizeof (filename));
 	else {
 		(void) fprintf(stderr, "A FILE must be specified.\n");
 		return (1);
@@ -1340,7 +1340,7 @@ draid_merge(int argc, char *argv[])
 		return (1);
 	}
 
-	strncpy(filename, argv[optind], MAXPATHLEN - 1);
+	strlcpy(filename, argv[optind], sizeof (filename));
 	optind++;
 
 	error = read_map(filename, &allcfgs);
@@ -1355,7 +1355,7 @@ draid_merge(int argc, char *argv[])
 		char srcfilename[MAXPATHLEN] = {0};
 		int merged = 0;
 
-		strncpy(srcfilename, argv[optind], MAXPATHLEN - 1);
+		strlcpy(srcfilename, argv[optind], sizeof (srcfilename));
 
 		error = draid_merge_impl(allcfgs, srcfilename, &merged);
 		if (error) {

tests/zfs-tests/cmd/zfs_diff-socket.c

@@ -37,8 +37,7 @@ main(int argc, char *argv[])
 	}
 	path = argv[1];
 	size =  sizeof (sock.sun_path);
-	strncpy(sock.sun_path, (char *)path, size - 1);
+	(void) snprintf(sock.sun_path, size, "%s", path);
-	sock.sun_path[size - 1] = '\0';
 
 	sock.sun_family = AF_UNIX;
 	if ((fd = socket(AF_UNIX, SOCK_DGRAM, 0)) == -1) {