aboutsummaryrefslogtreecommitdiffstats
path: root/module/zfs
diff options
context:
space:
mode:
authorRichard Yao <[email protected]>2022-09-27 19:35:29 -0400
committerGitHub <[email protected]>2022-09-27 16:35:29 -0700
commit7584fbe846b4127d5a16c5967a005e8f5c1e7b08 (patch)
tree75e37df0bbb0660c5cee6b3564a29e77fd04ae3e /module/zfs
parent3ed9d6883bcf3c55f92cdaaa6bf1aee2e6fb4115 (diff)
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 04a479f7066ccdaa23a6546955303b172f4a6909 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 <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #13876
Diffstat (limited to 'module/zfs')
-rw-r--r--module/zfs/dsl_dir.c6
-rw-r--r--module/zfs/dsl_prop.c6
-rw-r--r--module/zfs/spa_misc.c2
-rw-r--r--module/zfs/zcp_get.c3
-rw-r--r--module/zfs/zfs_ioctl.c2
-rw-r--r--module/zfs/zio_inject.c2
6 files changed, 9 insertions, 12 deletions
diff --git a/module/zfs/dsl_dir.c b/module/zfs/dsl_dir.c
index b32895946..d93c7f08c 100644
--- a/module/zfs/dsl_dir.c
+++ b/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);
- component[p - path] = '\0';
+ (void) strlcpy(component, path, p - path + 1);
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);
- component[p - path] = '\0';
+ (void) strlcpy(component, path, p - path + 1);
} else {
panic("invalid p=%p", (void *)p);
}
diff --git a/module/zfs/dsl_prop.c b/module/zfs/dsl_prop.c
index 1d3d26124..610e887b3 100644
--- a/module/zfs/dsl_prop.c
+++ b/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));
- buf[suffix - za.za_name] = '\0';
+ (void) strlcpy(buf, za.za_name,
+ MIN(sizeof (buf), suffix - za.za_name + 1));
propname = buf;
if (!(flags & DSL_PROP_GET_RECEIVED)) {
diff --git a/module/zfs/spa_misc.c b/module/zfs/spa_misc.c
index decf4ddae..daab1d6fc 100644
--- a/module/zfs/spa_misc.c
+++ b/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
diff --git a/module/zfs/zcp_get.c b/module/zfs/zcp_get.c
index 8230a4193..cd17374eb 100644
--- a/module/zfs/zcp_get.c
+++ b/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);
- domain_val[domain_len] = '\0';
+ (void) strlcpy(domain_val, cp, domain_len + 1);
cp += domain_len + 1;
(void) ddi_strtoll(cp, &end, 10, (longlong_t *)rid);
diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c
index 259d68c47..bafe6fe7d 100644
--- a/module/zfs/zfs_ioctl.c
+++ b/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';
diff --git a/module/zfs/zio_inject.c b/module/zfs/zio_inject.c
index 4f7cb8430..3598351c4 100644
--- a/module/zfs/zio_inject.c
+++ b/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);