Skip to content

Commit

Permalink
Fix TOCTOU race conditions reported by CodeQL and Coverity
Browse files Browse the repository at this point in the history
CodeQL and Coverity both complained about:

 * lib/libshare/os/linux/smb.c
 * tests/zfs-tests/cmd/mmapwrite.c
 	* twice
 * tests/zfs-tests/tests/functional/tmpfile/tmpfile_002_pos.c
 * tests/zfs-tests/tests/functional/tmpfile/tmpfile_stat_mode.c
	* coverity had a second complaint that CodeQL did not have
 * tests/zfs-tests/cmd/suid_write_to_file.c
	* Coverity had two complaints and CodeQL had one complaint, both
	  differed. The CodeQL complaint is about the main point of the
	  test, so it is not fixable without a hack involving `fork()`.

The issues reported by CodeQL are fixed, with the exception of the last
one, which is deemed to be a false positive that is too much trouble to
wrokaround. The issues reported by Coverity were only fixed if CodeQL
complained about them.

There were issues reported by Coverity in a number of other files that
were not reported by CodeQL, but fixing the CodeQL complaints is
considered a priority since we want to integrate it into a github
workflow, so the remaining Coverity complaints are left for future work.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#14098
  • Loading branch information
ryao authored and shodanshok committed Nov 3, 2022
1 parent a86c2e7 commit dff6f7f
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 29 deletions.
15 changes: 13 additions & 2 deletions lib/libshare/os/linux/smb.c
Original file line number Diff line number Diff line change
Expand Up @@ -90,21 +90,32 @@ smb_retrieve_shares(void)

/* Go through the directory, looking for shares */
while ((directory = readdir(shares_dir))) {
int fd;

if (directory->d_name[0] == '.')
continue;

snprintf(file_path, sizeof (file_path),
"%s/%s", SHARE_DIR, directory->d_name);

if ((fd = open(file_path, O_RDONLY | O_CLOEXEC)) == -1) {
rc = SA_SYSTEM_ERR;
goto out;
}

if (stat(file_path, &eStat) == -1) {
close(fd);
rc = SA_SYSTEM_ERR;
goto out;
}

if (!S_ISREG(eStat.st_mode))
if (!S_ISREG(eStat.st_mode)) {
close(fd);
continue;
}

if ((share_file_fp = fopen(file_path, "re")) == NULL) {
if ((share_file_fp = fdopen(fd, "r")) == NULL) {
close(fd);
rc = SA_SYSTEM_ERR;
goto out;
}
Expand Down
14 changes: 3 additions & 11 deletions tests/zfs-tests/cmd/mmapwrite.c
Original file line number Diff line number Diff line change
Expand Up @@ -88,29 +88,21 @@ map_writer(void *filename)
int ret = 0;
char *buf = NULL;
int page_size = getpagesize();
int op_errno = 0;
char *file_path = filename;

while (1) {
ret = access(file_path, F_OK);
if (ret) {
op_errno = errno;
if (op_errno == ENOENT) {
fd = open(file_path, O_RDWR);
if (fd == -1) {
if (errno == ENOENT) {
fd = open(file_path, O_RDWR | O_CREAT, 0777);
if (fd == -1) {
err(1, "open file failed");
}

ret = ftruncate(fd, page_size);
if (ret == -1) {
err(1, "truncate file failed");
}
} else {
err(1, "access file failed!");
}
} else {
fd = open(file_path, O_RDWR, 0777);
if (fd == -1) {
err(1, "open file failed");
}
}
Expand Down
7 changes: 2 additions & 5 deletions tests/zfs-tests/tests/functional/tmpfile/tmpfile_002_pos.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ main(void)
int i, fd;
char spath[1024], dpath[1024];
const char *penv[] = {"TESTDIR", "TESTFILE0"};
struct stat sbuf;

(void) fprintf(stdout, "Verify O_TMPFILE file can be linked.\n");

Expand All @@ -73,12 +72,10 @@ main(void)
run("export");
run("import");

if (stat(dpath, &sbuf) < 0) {
perror("stat");
unlink(dpath);
if (unlink(dpath) == -1) {
perror("unlink");
exit(5);
}
unlink(dpath);

return (0);
}
25 changes: 14 additions & 11 deletions tests/zfs-tests/tests/functional/tmpfile/tmpfile_stat_mode.c
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
static void
test_stat_mode(mode_t mask)
{
struct stat st, fst;
struct stat fst;
int i, fd;
char spath[1024], dpath[1024];
const char *penv[] = {"TESTDIR", "TESTFILE0"};
Expand All @@ -68,7 +68,7 @@ test_stat_mode(mode_t mask)
err(2, "open(%s)", penv[0]);

if (fstat(fd, &fst) == -1)
err(3, "open");
err(3, "fstat(%s)", penv[0]);

snprintf(spath, sizeof (spath), "/proc/self/fd/%d", fd);
snprintf(dpath, sizeof (dpath), "%s/%s", penv[0], penv[1]);
Expand All @@ -78,19 +78,22 @@ test_stat_mode(mode_t mask)
err(4, "linkat");
close(fd);

if (stat(dpath, &st) == -1)
err(5, "stat");
unlink(dpath);

/* Verify fstat(2) result */
/* Verify fstat(2) result at old path */
mode = fst.st_mode & 0777;
if (mode != masked)
errx(6, "fstat(2) %o != %o\n", mode, masked);
errx(5, "fstat(2) %o != %o\n", mode, masked);

fd = open(dpath, O_RDWR);
if (fd == -1)
err(6, "open(%s)", dpath);

/* Verify stat(2) result */
mode = st.st_mode & 0777;
if (fstat(fd, &fst) == -1)
err(7, "fstat(%s)", dpath);

/* Verify fstat(2) result at new path */
mode = fst.st_mode & 0777;
if (mode != masked)
errx(7, "stat(2) %o != %o\n", mode, masked);
errx(8, "fstat(2) %o != %o\n", mode, masked);
}

int
Expand Down

0 comments on commit dff6f7f

Please sign in to comment.