aboutsummaryrefslogtreecommitdiffstats
path: root/contrib/pam_zfs_key
diff options
context:
space:
mode:
authorRichard Yao <[email protected]>2022-09-27 19:47:24 -0400
committerGitHub <[email protected]>2022-09-27 16:47:24 -0700
commita51288aabbbc176a8a73a8b3cd56f79607db32cf (patch)
tree7b7570d922e6ea1885cdb593d09826cc18e38fd5 /contrib/pam_zfs_key
parent88b199c24e789f3680193d2f41101f75efd8803f (diff)
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 <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #13913
Diffstat (limited to 'contrib/pam_zfs_key')
-rw-r--r--contrib/pam_zfs_key/pam_zfs_key.c5
1 files changed, 2 insertions, 3 deletions
diff --git a/contrib/pam_zfs_key/pam_zfs_key.c b/contrib/pam_zfs_key/pam_zfs_key.c
index aaca67008..e0bbd249a 100644
--- a/contrib/pam_zfs_key/pam_zfs_key.c
+++ b/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);
}