From ab32a14b2ed7531a76fd13a5278dd7d47d2ee923 Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Thu, 20 Oct 2022 17:46:12 -0400 Subject: [PATCH] Silence new static analyzer defect reports from idmap_util.c 2a068a1394d179dda4becf350e3afb4e8819675e introduced 2 new defect reports from Coverity and 1 from Clang's static analyzer. Coverity complained about a potential resource leak from only calling `close(fd)` when `fd > 0` because `fd` might be `0`. This is a false positive, but rather than dismiss it as such, we can change the comparison to ensure that this never appears again from any static analyzer. Upon inspection, 6 more instances of this were found in the file, so those were changed too. Unfortunately, since the file descriptor has been put into an unsigned variable in `attr.userns_fd`, we cannot do a non-negative check on it to see if it has not been allocated, so we instead restructure the error handling to avoid the need for a check. This also means that errors had not been handled correctly here, so the static analyzer found a bug (although practically by accident). Coverity also complained about a dereference before a NULL check in `do_idmap_mount()` on `source`. Upon inspection, it appears that the pointer is never NULL, so we delete the NULL check as cleanup. Clang's static analyzer complained that the return value of `write_pid_idmaps()` can be uninitialized if we have no idmaps to write. Reviewed-by: Brian Behlendorf Reviewed-by: Youzhong Yang Signed-off-by: Richard Yao Closes #14061 --- tests/zfs-tests/cmd/idmap_util.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/tests/zfs-tests/cmd/idmap_util.c b/tests/zfs-tests/cmd/idmap_util.c index 143cd4db67..b6ca111ad2 100644 --- a/tests/zfs-tests/cmd/idmap_util.c +++ b/tests/zfs-tests/cmd/idmap_util.c @@ -316,7 +316,7 @@ write_idmap(pid_t pid, char *buf, size_t buf_size, idmap_type_t type) else ret = 0; out: - if (fd > 0) + if (fd >= 0) close(fd); return (ret); } @@ -339,7 +339,7 @@ write_pid_idmaps(pid_t pid, list_t *head) int size_buf_uids = 4096, size_buf_gids = 4096; struct idmap_entry *entry; int uid_filled, gid_filled; - int ret; + int ret = 0; int has_uids = 0, has_gids = 0; size_t buf_size; @@ -546,11 +546,11 @@ is_idmap_supported(char *path) if (ret) { errno = ret; log_errno("parse_idmap_entry(%s)", input); - goto out; + goto out1; } ret = userns_fd_from_idmap(&head); if (ret < 0) - goto out; + goto out1; attr.userns_fd = ret; ret = openat(-EBADF, path, O_DIRECTORY | O_CLOEXEC); if (ret < 0) { @@ -571,14 +571,14 @@ is_idmap_supported(char *path) log_errno("sys_mount_setattr"); } out: + close(attr.userns_fd); +out1: free_idmap(&head); list_destroy(&head); - if (tree_fd > 0) + if (tree_fd >= 0) close(tree_fd); - if (path_fd > 0) + if (path_fd >= 0) close(path_fd); - if (attr.userns_fd > 0) - close(attr.userns_fd); free(input); return (ret == 0); } @@ -639,7 +639,7 @@ do_idmap_mount(list_t *idmap, char *source, char *target, int flags) ret = userns_fd_from_idmap(idmap); if (ret < 0) - goto out; + goto out1; attr.userns_fd = ret; ret = openat(-EBADF, source, O_DIRECTORY | O_CLOEXEC); if (ret < 0) { @@ -663,7 +663,7 @@ do_idmap_mount(list_t *idmap, char *source, char *target, int flags) log_errno("sys_mount_setattr"); goto out; } - if (source != NULL && target == NULL && is_mountpoint(source)) { + if (target == NULL && is_mountpoint(source)) { ret = umount2(source, MNT_DETACH); if (ret < 0) { ret = -errno; @@ -679,12 +679,12 @@ do_idmap_mount(list_t *idmap, char *source, char *target, int flags) source : target); } out: - if (tree_fd > 0) + close(attr.userns_fd); +out1: + if (tree_fd >= 0) close(tree_fd); - if (source_fd > 0) + if (source_fd >= 0) close(source_fd); - if (attr.userns_fd > 0) - close(attr.userns_fd); return (ret); }