Archive-Team
/
zfs
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases 103 Wiki Activity

Silence new static analyzer defect reports from idmap_util.c 

2a068a1394 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 <behlendorf1@llnl.gov>
Reviewed-by: Youzhong Yang <yyang@mathworks.com>
Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Closes #14061
This commit is contained in:
Richard Yao 2022-10-20 17:46:12 -04:00 committed by GitHub
parent a06df8d7c1
commit ab32a14b2e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
1 changed files with 14 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
tests/zfs-tests/cmd/idmap_util.c
View File

@ -316,7 +316,7 @@ write_idmap(pid_t pid, char *buf, size_t buf_size, idmap_type_t type)
else else
ret = 0; ret = 0;
out: out:
if (fd > 0) if (fd >= 0)
close(fd); close(fd);
return (ret); return (ret);
} }
@ -339,7 +339,7 @@ write_pid_idmaps(pid_t pid, list_t *head)
int size_buf_uids = 4096, size_buf_gids = 4096; int size_buf_uids = 4096, size_buf_gids = 4096;
struct idmap_entry *entry; struct idmap_entry *entry;
int uid_filled, gid_filled; int uid_filled, gid_filled;
int ret; int ret = 0;
int has_uids = 0, has_gids = 0; int has_uids = 0, has_gids = 0;
size_t buf_size; size_t buf_size;
@ -546,11 +546,11 @@ is_idmap_supported(char *path)
if (ret) { if (ret) {
errno = ret; errno = ret;
log_errno("parse_idmap_entry(%s)", input); log_errno("parse_idmap_entry(%s)", input);
goto out; goto out1;
} }
ret = userns_fd_from_idmap(&head); ret = userns_fd_from_idmap(&head);
if (ret < 0) if (ret < 0)
goto out; goto out1;
attr.userns_fd = ret; attr.userns_fd = ret;
ret = openat(-EBADF, path, O_DIRECTORY | O_CLOEXEC); ret = openat(-EBADF, path, O_DIRECTORY | O_CLOEXEC);
if (ret < 0) { if (ret < 0) {
@ -571,14 +571,14 @@ is_idmap_supported(char *path)
log_errno("sys_mount_setattr"); log_errno("sys_mount_setattr");
} }
out: out:
close(attr.userns_fd);
out1:
free_idmap(&head); free_idmap(&head);
list_destroy(&head); list_destroy(&head);
if (tree_fd > 0) if (tree_fd >= 0)
close(tree_fd); close(tree_fd);
if (path_fd > 0) if (path_fd >= 0)
close(path_fd); close(path_fd);
if (attr.userns_fd > 0)
close(attr.userns_fd);
free(input); free(input);
return (ret == 0); 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); ret = userns_fd_from_idmap(idmap);
if (ret < 0) if (ret < 0)
goto out; goto out1;
attr.userns_fd = ret; attr.userns_fd = ret;
ret = openat(-EBADF, source, O_DIRECTORY | O_CLOEXEC); ret = openat(-EBADF, source, O_DIRECTORY | O_CLOEXEC);
if (ret < 0) { if (ret < 0) {
@ -663,7 +663,7 @@ do_idmap_mount(list_t *idmap, char *source, char *target, int flags)
log_errno("sys_mount_setattr"); log_errno("sys_mount_setattr");
goto out; goto out;
} }
if (source != NULL && target == NULL && is_mountpoint(source)) { if (target == NULL && is_mountpoint(source)) {
ret = umount2(source, MNT_DETACH); ret = umount2(source, MNT_DETACH);
if (ret < 0) { if (ret < 0) {
ret = -errno; ret = -errno;
@ -679,12 +679,12 @@ do_idmap_mount(list_t *idmap, char *source, char *target, int flags)
source : target); source : target);
} }
out: out:
if (tree_fd > 0) close(attr.userns_fd);
out1:
if (tree_fd >= 0)
close(tree_fd); close(tree_fd);
if (source_fd > 0) if (source_fd >= 0)
close(source_fd); close(source_fd);
if (attr.userns_fd > 0)
close(attr.userns_fd);
return (ret); return (ret);
} }