]> xenbits.xensource.com Git - freebsd.git/commitdiff
fusefs: coverity cleanup in the tests
authorasomers <asomers@FreeBSD.org>
Fri, 6 Sep 2019 19:50:45 +0000 (19:50 +0000)
committerasomers <asomers@FreeBSD.org>
Fri, 6 Sep 2019 19:50:45 +0000 (19:50 +0000)
Address the following defects reported by Coverity:

* Structurally dead code (CID 1404366): set m_quit before FAIL, not after

* Unchecked return value of sysctlbyname (CID 1404321)

* Unchecked return value of stat(2) (CID 1404471)

* Unchecked return value of open(2) (CID 14044021404529)

* Unchecked return value of dup(2) (CID 1404478)

* Buffer overflows. These are all false positives caused by the fact that
  Coverity thinks I'm using a buffer to store strings, when in fact I'm
  really just using it to store a byte array that happens to be initialized
  with a string. I'm changing the type from char to uint8_t in the hopes
  that it will placate Coverity. (CID 1404338140435014043671404376,
  14043791404381140438814044031404425140443314044341404474,
  1404480140448414045031404505)

* False positive file descriptor leak. I'm going to try to fix this with
  Coverity modeling, but I'll also change an EXPECT to ASSERT so we don't
  perform meaningless assertions after the failure. (CID 14043201404324,
  14044401404445).

* Unannotated file descriptor leak. This will be followed up by a Coverity
  modeling change. (CID 14043261404334140433614043571404361,
  14043721404391140439514044091404430140444814044511404455,
  140445714044581404460)

* Uninitialized variables in C++ constructors (CID 14043271404346). In the
  case of m_maxphys, this actually led to part of the FUSE_INIT's response
  being set to stack garbage during the WriteCluster::clustering test.

* Uninitialized sun_len field in struct sockaddr_un (CID 14043301404371,
  1404429).

Reported by: Coverity
Reviewed by: emaste
MFC after: 3 days
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D21457

18 files changed:
tests/sys/fs/fusefs/allow_other.cc
tests/sys/fs/fusefs/bmap.cc
tests/sys/fs/fusefs/default_permissions.cc
tests/sys/fs/fusefs/dev_fuse_poll.cc
tests/sys/fs/fusefs/fifo.cc
tests/sys/fs/fusefs/interrupt.cc
tests/sys/fs/fusefs/io.cc
tests/sys/fs/fusefs/mknod.cc
tests/sys/fs/fusefs/mockfs.cc
tests/sys/fs/fusefs/notify.cc
tests/sys/fs/fusefs/open.cc
tests/sys/fs/fusefs/opendir.cc
tests/sys/fs/fusefs/read.cc
tests/sys/fs/fusefs/release.cc
tests/sys/fs/fusefs/setattr.cc
tests/sys/fs/fusefs/utils.cc
tests/sys/fs/fusefs/utils.hh
tests/sys/fs/fusefs/write.cc

index 43f1f43bca08d382de7e5dcc5889a4b76993226b..0281a9262401554ec65860b170b6d8a511527dc8 100644 (file)
@@ -255,6 +255,8 @@ TEST_F(NoAllowOther, disallowed_beneath_root)
                }
        );
        ASSERT_EQ(0, WEXITSTATUS(status));
+
+       leak(dfd);
 }
 
 /* 
index c7bc719febf1864c9a679fcc2f49dc9f2bd3e709..b41aeb4f08dd03fe2bc3a91206619ff074ac2747 100644 (file)
@@ -158,4 +158,6 @@ TEST_F(Bmap, default_)
        EXPECT_EQ(arg.bn, lbn * m_maxbcachebuf / DEV_BSIZE);
        EXPECT_EQ(arg.runp, 0);
        EXPECT_EQ(arg.runb, m_maxphys / m_maxbcachebuf - 1);
+
+       leak(fd);
 }
index c6dcd4f0f3560b92af8c936accfc9bf9eda132cd..dbb0d2e450272bf56fd72924a055e632b1a9c8db 100644 (file)
@@ -503,7 +503,7 @@ TEST_F(Create, eacces)
        EXPECT_LOOKUP(FUSE_ROOT_ID, RELPATH)
                .WillOnce(Invoke(ReturnErrno(ENOENT)));
 
-       EXPECT_EQ(-1, open(FULLPATH, O_CREAT | O_EXCL, 0644));
+       ASSERT_EQ(-1, open(FULLPATH, O_CREAT | O_EXCL, 0644));
        EXPECT_EQ(EACCES, errno);
 }
 
index 48bbc8dd0d908b370db4e5715a0c58dac19fd2f0..951777701e1e54986b4f2f6d8d970644dac8f667 100644 (file)
@@ -107,13 +107,12 @@ static void* statter(void* arg) {
        struct stat sb;
 
        name = (const char*)arg;
-       stat(name, &sb);
-       return 0;
+       return ((void*)(intptr_t)stat(name, &sb));
 }
 
 /*
  * A kevent's data field should contain the number of operations available to
- * be immediately rea.
+ * be immediately read.
  */
 TEST_F(Kqueue, data)
 {
@@ -124,6 +123,7 @@ TEST_F(Kqueue, data)
        uint64_t bar_ino = 43;
        uint64_t baz_ino = 44;
        Sequence seq;
+       void *th_ret;
 
        ASSERT_EQ(0, sem_init(&sem0, 0, 0)) << strerror(errno);
        ASSERT_EQ(0, sem_init(&sem1, 0, 0)) << strerror(errno);
@@ -216,9 +216,12 @@ TEST_F(Kqueue, data)
        nap();          // Allow th1 and th2 to send their ops to the daemon
        EXPECT_EQ(0, sem_post(&sem1)) << strerror(errno);
 
-       pthread_join(th0, NULL);
-       pthread_join(th1, NULL);
-       pthread_join(th2, NULL);
+       pthread_join(th0, &th_ret);
+       ASSERT_EQ(-1, (intptr_t)th_ret);
+       pthread_join(th1, &th_ret);
+       ASSERT_EQ(-1, (intptr_t)th_ret);
+       pthread_join(th2, &th_ret);
+       ASSERT_EQ(-1, (intptr_t)th_ret);
 
        EXPECT_EQ(1, nready0);
        EXPECT_EQ(2, nready1);
index 7ab00513696534775c99611617fd16bfbf5e723b..bbaacb1285d64d1263adc4ed0146a54834591015 100644 (file)
@@ -124,6 +124,7 @@ static void* socket_writer(void* arg __unused) {
        }
        sa.sun_family = AF_UNIX;
        strlcpy(sa.sun_path, FULLPATH, sizeof(sa.sun_path));
+       sa.sun_len = sizeof(FULLPATH);
        err = connect(fd, (struct sockaddr*)&sa, sizeof(sa));
        if (err < 0) {
                perror("connect");
@@ -140,6 +141,8 @@ static void* socket_writer(void* arg __unused) {
                        sent += r;
                
        }
+
+       FuseTest::leak(fd);
        return 0;
 }
 
@@ -189,6 +192,7 @@ TEST_F(Socket, read_write)
        ASSERT_LE(0, fd) << strerror(errno);
        sa.sun_family = AF_UNIX;
        strlcpy(sa.sun_path, FULLPATH, sizeof(sa.sun_path));
+       sa.sun_len = sizeof(FULLPATH);
        ASSERT_EQ(0, bind(fd, (struct sockaddr*)&sa, sizeof(sa)))
                << strerror(errno);
        listen(fd, 5);
index d38a9c30101338232451db9ead00c41d39cb63fb..6893bea077e780cd080f7d160b2c9e04cde637db 100644 (file)
@@ -466,6 +466,8 @@ TEST_F(Intr, in_kernel_restartable)
        EXPECT_EQ(0, (intptr_t)thr0_value);
        sem_destroy(&sem1);
        sem_destroy(&sem0);
+
+       leak(fd1);
 }
 
 /*
@@ -536,6 +538,8 @@ TEST_F(Intr, in_kernel_nonrestartable)
        EXPECT_EQ(0, (intptr_t)thr0_value);
        sem_destroy(&sem1);
        sem_destroy(&sem0);
+
+       leak(fd1);
 }
 
 /* 
@@ -611,6 +615,8 @@ TEST_F(Intr, in_progress_read)
        setup_interruptor(self);
        ASSERT_EQ(-1, read(fd, buf, bufsize));
        EXPECT_EQ(EINTR, errno);
+
+       leak(fd);
 }
 
 /*
index d94788eb6e44e6df6fb458323c798efa3465aeb4..135fc4c9194b05bef17b983be86738d8841a3f96 100644 (file)
@@ -108,7 +108,7 @@ int m_backing_fd, m_control_fd, m_test_fd;
 off_t m_filesize;
 bool m_direct_io;
 
-Io(): m_backing_fd(-1), m_control_fd(-1), m_direct_io(false) {};
+Io(): m_backing_fd(-1), m_control_fd(-1), m_test_fd(-1), m_direct_io(false) {};
 
 void SetUp()
 {
index 515d233dbeba1a5935bf9b67a873aadb1b1c3b21..53087b75d987209d885af2261b48ac2e2cac7c6a 100644 (file)
@@ -214,8 +214,11 @@ TEST_F(Mknod, socket)
        ASSERT_LE(0, fd) << strerror(errno);
        sa.sun_family = AF_UNIX;
        strlcpy(sa.sun_path, FULLPATH, sizeof(sa.sun_path));
+       sa.sun_len = sizeof(FULLPATH);
        ASSERT_EQ(0, bind(fd, (struct sockaddr*)&sa, sizeof(sa)))
                << strerror(errno);
+
+       leak(fd);
 }
 
 /* 
index d1042142c589f5a917df11cb5441c7b16f60148b..ee0c8fb99d01a2c9f8f21b0480edb5eadc75ad64 100644 (file)
@@ -833,8 +833,8 @@ void MockFS::read_request(mockfs_buf_in &in) {
        res = read(m_fuse_fd, &in, sizeof(in));
 
        if (res < 0 && !m_quit) {
-               FAIL() << "read: " << strerror(errno);
                m_quit = true;
+               FAIL() << "read: " << strerror(errno);
        }
        ASSERT_TRUE(res >= static_cast<ssize_t>(sizeof(in.header)) || m_quit);
        /*
index 694a3ba812f12bf031be82ec81a8755e0fcf0ab0..4df0aff61b2ce0d627ee467d83e30865ec7fe516 100644 (file)
@@ -465,6 +465,7 @@ TEST_F(NotifyWriteback, inval_inode_with_dirty_cache)
 
        /* Fill the data cache */
        fd = open(FULLPATH, O_RDWR);
+       ASSERT_LE(0, fd);
        ASSERT_EQ(bufsize, write(fd, CONTENTS, bufsize)) << strerror(errno);
 
        expect_write(ino, 0, bufsize, CONTENTS);
@@ -526,6 +527,7 @@ TEST_F(NotifyWriteback, inval_inode_attrs_only)
 
        /* Fill the data cache */
        fd = open(FULLPATH, O_RDWR);
+       ASSERT_LE(0, fd) << strerror(errno);
        ASSERT_EQ(bufsize, write(fd, CONTENTS, bufsize)) << strerror(errno);
 
        /* Evict the attributes, but not data cache */
index bc391a7024a7a80ef768dd22403dd7356ca336b1..8aef5bedf2ec1e2cca36624a081584ca75679e20 100644 (file)
@@ -114,7 +114,7 @@ TEST_F(Open, enoent)
                }, Eq(true)),
                _)
        ).WillOnce(Invoke(ReturnErrno(ENOENT)));
-       EXPECT_NE(0, open(FULLPATH, O_RDONLY));
+       ASSERT_EQ(-1, open(FULLPATH, O_RDONLY));
        EXPECT_EQ(ENOENT, errno);
 }
 
@@ -136,7 +136,7 @@ TEST_F(Open, eperm)
                }, Eq(true)),
                _)
        ).WillOnce(Invoke(ReturnErrno(EPERM)));
-       EXPECT_NE(0, open(FULLPATH, O_RDONLY));
+       ASSERT_EQ(-1, open(FULLPATH, O_RDONLY));
        EXPECT_EQ(EPERM, errno);
 }
 
index 47b667339b62f9c111e5c808e9f6292097575995..ff3730a055a29fc2bf89d3226f19844e3211b199 100644 (file)
@@ -86,7 +86,7 @@ TEST_F(Opendir, enoent)
        expect_lookup(RELPATH, ino);
        expect_opendir(ino, O_RDONLY, ReturnErrno(ENOENT));
 
-       EXPECT_NE(0, open(FULLPATH, O_DIRECTORY));
+       ASSERT_EQ(-1, open(FULLPATH, O_DIRECTORY));
        EXPECT_EQ(ENOENT, errno);
 }
 
@@ -112,6 +112,7 @@ TEST_F(Opendir, open)
        const char FULLPATH[] = "mountpoint/some_dir";
        const char RELPATH[] = "some_dir";
        uint64_t ino = 42;
+       int fd;
 
        expect_lookup(RELPATH, ino);
        expect_opendir(ino, O_RDONLY,
@@ -119,7 +120,10 @@ TEST_F(Opendir, open)
                SET_OUT_HEADER_LEN(out, open);
        }));
 
-       EXPECT_LE(0, open(FULLPATH, O_DIRECTORY)) << strerror(errno);
+       fd = open(FULLPATH, O_DIRECTORY);
+       EXPECT_LE(0, fd) << strerror(errno);
+
+       leak(fd);
 }
 
 /* Directories can be opened O_EXEC for stuff like fchdir(2) */
@@ -138,6 +142,8 @@ TEST_F(Opendir, open_exec)
 
        fd = open(FULLPATH, O_EXEC | O_DIRECTORY);
        ASSERT_LE(0, fd) << strerror(errno);
+
+       leak(fd);
 }
 
 TEST_F(Opendir, opendir)
index 7d8220d78111177535c2c67b1446d405ebaaf601..a333b9f15a3534ac28d8a5f68ec4dff4d9ffe116 100644 (file)
@@ -113,7 +113,7 @@ TEST_F(AioRead, aio_read)
        uint64_t ino = 42;
        int fd;
        ssize_t bufsize = strlen(CONTENTS);
-       char buf[bufsize];
+       uint8_t buf[bufsize];
        struct aiocb iocb, *piocb;
 
        expect_lookup(RELPATH, ino, bufsize);
@@ -327,7 +327,7 @@ TEST_F(Read, direct_io_pread)
        int fd;
        uint64_t offset = 100;
        ssize_t bufsize = strlen(CONTENTS);
-       char buf[bufsize];
+       uint8_t buf[bufsize];
 
        expect_lookup(RELPATH, ino, offset + bufsize);
        expect_open(ino, FOPEN_DIRECT_IO, 1);
@@ -361,7 +361,7 @@ TEST_F(Read, direct_io_short_read)
        uint64_t offset = 100;
        ssize_t bufsize = strlen(CONTENTS);
        ssize_t halfbufsize = bufsize / 2;
-       char buf[bufsize];
+       uint8_t buf[bufsize];
 
        expect_lookup(RELPATH, ino, offset + bufsize);
        expect_open(ino, FOPEN_DIRECT_IO, 1);
@@ -384,7 +384,7 @@ TEST_F(Read, eio)
        uint64_t ino = 42;
        int fd;
        ssize_t bufsize = strlen(CONTENTS);
-       char buf[bufsize];
+       uint8_t buf[bufsize];
 
        expect_lookup(RELPATH, ino, bufsize);
        expect_open(ino, 0, 1);
@@ -419,7 +419,7 @@ TEST_F(Read, eof)
        ssize_t bufsize = strlen(CONTENTS);
        ssize_t partbufsize = 3 * bufsize / 4;
        ssize_t r;
-       char buf[bufsize];
+       uint8_t buf[bufsize];
        struct stat sb;
 
        expect_lookup(RELPATH, ino, offset + bufsize);
@@ -448,7 +448,7 @@ TEST_F(Read, eof_of_whole_buffer)
        int fd;
        ssize_t bufsize = strlen(CONTENTS);
        off_t old_filesize = m_maxbcachebuf * 2 + bufsize;
-       char buf[bufsize];
+       uint8_t buf[bufsize];
        struct stat sb;
 
        expect_lookup(RELPATH, ino, old_filesize);
@@ -483,7 +483,7 @@ TEST_F(Read, keep_cache)
        uint64_t ino = 42;
        int fd0, fd1;
        ssize_t bufsize = strlen(CONTENTS);
-       char buf[bufsize];
+       uint8_t buf[bufsize];
 
        FuseTest::expect_lookup(RELPATH, ino, S_IFREG | 0644, bufsize, 2);
        expect_open(ino, FOPEN_KEEP_CACHE, 2);
@@ -518,7 +518,7 @@ TEST_F(Read, keep_cache_disabled)
        uint64_t ino = 42;
        int fd0, fd1;
        ssize_t bufsize = strlen(CONTENTS);
-       char buf[bufsize];
+       uint8_t buf[bufsize];
 
        FuseTest::expect_lookup(RELPATH, ino, S_IFREG | 0644, bufsize, 2);
        expect_open(ino, 0, 2);
@@ -646,7 +646,7 @@ TEST_F(Read, o_direct)
        uint64_t ino = 42;
        int fd;
        ssize_t bufsize = strlen(CONTENTS);
-       char buf[bufsize];
+       uint8_t buf[bufsize];
 
        expect_lookup(RELPATH, ino, bufsize);
        expect_open(ino, 0, 1);
@@ -682,7 +682,7 @@ TEST_F(Read, pread)
         */
        uint64_t offset = m_maxbcachebuf;
        ssize_t bufsize = strlen(CONTENTS);
-       char buf[bufsize];
+       uint8_t buf[bufsize];
 
        expect_lookup(RELPATH, ino, offset + bufsize);
        expect_open(ino, 0, 1);
@@ -704,7 +704,7 @@ TEST_F(Read, read)
        uint64_t ino = 42;
        int fd;
        ssize_t bufsize = strlen(CONTENTS);
-       char buf[bufsize];
+       uint8_t buf[bufsize];
 
        expect_lookup(RELPATH, ino, bufsize);
        expect_open(ino, 0, 1);
@@ -727,7 +727,7 @@ TEST_F(Read_7_8, read)
        uint64_t ino = 42;
        int fd;
        ssize_t bufsize = strlen(CONTENTS);
-       char buf[bufsize];
+       uint8_t buf[bufsize];
 
        expect_lookup(RELPATH, ino, bufsize);
        expect_open(ino, 0, 1);
@@ -789,7 +789,7 @@ TEST_F(Read, sendfile)
        uint64_t ino = 42;
        int fd;
        size_t bufsize = strlen(CONTENTS);
-       char buf[bufsize];
+       uint8_t buf[bufsize];
        int sp[2];
        off_t sbytes;
 
index f737a5c6655e1d91979adb4b07ac7ed2f70ec9c3..8953f4da15df5698361dbba9ad390795f8c21713 100644 (file)
@@ -90,6 +90,7 @@ TEST_F(Release, dup)
        EXPECT_LE(0, fd) << strerror(errno);
 
        fd2 = dup(fd);
+       ASSERT_LE(0, fd2) << strerror(errno);
 
        ASSERT_EQ(0, close(fd2)) << strerror(errno);
        ASSERT_EQ(0, close(fd)) << strerror(errno);
index 94ad1703b16ceb3641ebda76c3b6568d86d2120f..163d7a3d893034d5a98f3004110684eb4e610d59 100644 (file)
@@ -548,6 +548,8 @@ TEST_F(Setattr, truncate_discards_cached_data) {
        free(r1buf);
        free(r0buf);
        free(w0buf);
+
+       leak(fd);
 }
 
 /* Change a file's timestamps */
index af714620d2f25cd953bb5f124f2388da3c1bb5e0..cc160d2e2804efd735ff4a0463a9d1a495b95684 100644 (file)
@@ -83,8 +83,9 @@ void check_environment()
                        GTEST_SKIP() << strerror(errno);
                }
        }
-       sysctlbyname(usermount_node, &usermount_val, &usermount_size,
-                    NULL, 0);
+       ASSERT_EQ(sysctlbyname(usermount_node, &usermount_val, &usermount_size,
+                              NULL, 0),
+                 0);;
        if (geteuid() != 0 && !usermount_val)
                GTEST_SKIP() << "current user is not allowed to mount";
 }
index babc6a33ad1b89c03a1a1090d47955bf09992ad2..85d4d81b3ec34f55722a10dbcfa0df0ef6a62eb1 100644 (file)
@@ -83,7 +83,9 @@ class FuseTest : public ::testing::Test {
                m_async(false),
                m_noclusterr(false),
                m_nointr(false),
-               m_time_gran(1)
+               m_time_gran(1),
+               m_maxbcachebuf(0),
+               m_maxphys(0)
        {}
 
        virtual void SetUp();
index 43f925a96e92919f5186f6f6fb78d456d95b2d4e..4b4a2f7c4f39eade52885a1f01c397ccec6ff73d 100644 (file)
@@ -183,7 +183,7 @@ class WriteCluster: public WriteBack {
 public:
 virtual void SetUp() {
        m_async = true;
-       m_maxwrite = m_maxphys;
+       m_maxwrite = 1 << 25;   // Anything larger than MAXPHYS will suffice
        WriteBack::SetUp();
        if (m_maxphys < 2 * DFLTPHYS)
                GTEST_SKIP() << "MAXPHYS must be at least twice DFLTPHYS"
@@ -563,6 +563,8 @@ TEST_F(Write, mmap)
 
        free(expected);
        free(zeros);
+
+       leak(fd);
 }
 
 TEST_F(Write, pwrite)
@@ -614,6 +616,8 @@ TEST_F(Write, timestamps)
        EXPECT_EQ(sb0.st_atime, sb1.st_atime);
        EXPECT_NE(sb0.st_mtime, sb1.st_mtime);
        EXPECT_NE(sb0.st_ctime, sb1.st_ctime);
+
+       leak(fd);
 }
 
 TEST_F(Write, write)
@@ -863,7 +867,7 @@ TEST_F(WriteBack, cache)
        uint64_t ino = 42;
        int fd;
        ssize_t bufsize = strlen(CONTENTS);
-       char readbuf[bufsize];
+       uint8_t readbuf[bufsize];
 
        expect_lookup(RELPATH, ino, 0);
        expect_open(ino, 0, 1);
@@ -895,7 +899,7 @@ TEST_F(WriteBack, o_direct)
        uint64_t ino = 42;
        int fd;
        ssize_t bufsize = strlen(CONTENTS);
-       char readbuf[bufsize];
+       uint8_t readbuf[bufsize];
 
        expect_lookup(RELPATH, ino, 0);
        expect_open(ino, 0, 1);
@@ -942,6 +946,7 @@ TEST_F(WriteBackAsync, delay)
        ASSERT_EQ(bufsize, write(fd, CONTENTS, bufsize)) << strerror(errno);
 
        /* Don't close the file because that would flush the cache */
+       leak(fd);
 }
 
 /*
@@ -1175,6 +1180,8 @@ TEST_F(WriteBackAsync, timestamps)
        EXPECT_EQ((time_t)server_time, sb.st_atime);
        EXPECT_NE((time_t)server_time, sb.st_mtime);
        EXPECT_NE((time_t)server_time, sb.st_ctime);
+
+       leak(fd);
 }
 
 /* Any dirty timestamp fields should be flushed during a SETATTR */
@@ -1208,6 +1215,8 @@ TEST_F(WriteBackAsync, timestamps_during_setattr)
        EXPECT_LE(0, fd) << strerror(errno);
        ASSERT_EQ(bufsize, write(fd, CONTENTS, bufsize)) << strerror(errno);
        ASSERT_EQ(0, fchmod(fd, newmode)) << strerror(errno);
+
+       leak(fd);
 }
 
 /* fuse_init_out.time_gran controls the granularity of timestamps */
@@ -1243,6 +1252,8 @@ TEST_P(TimeGran, timestamps_during_setattr)
        EXPECT_LE(0, fd) << strerror(errno);
        ASSERT_EQ(bufsize, write(fd, CONTENTS, bufsize)) << strerror(errno);
        ASSERT_EQ(0, fchmod(fd, newmode)) << strerror(errno);
+
+       leak(fd);
 }
 
 INSTANTIATE_TEST_CASE_P(RA, TimeGran, Range(0u, 10u));
@@ -1258,7 +1269,7 @@ TEST_F(Write, writethrough)
        uint64_t ino = 42;
        int fd;
        ssize_t bufsize = strlen(CONTENTS);
-       char readbuf[bufsize];
+       uint8_t readbuf[bufsize];
 
        expect_lookup(RELPATH, ino, 0);
        expect_open(ino, 0, 1);