Reduce false positives from Static Analyzers

Both Clang's Static Analyzer and Synopsys' Coverity would ignore
assertions. Following Clang's advice, we annotate our assertions:

https://clang-analyzer.llvm.org/annotations.html#custom_assertions

This makes both Clang's Static Analyzer and Coverity properly identify
assertions. This change reduced Clang's reported defects from 246 to
180. It also reduced the false positives reported by Coverityi by 10,
while enabling Coverity to find 9 more defects that previously were
false negatives.

A couple examples of this would be CID-1524417 and CID-1524423. After
submitting a build to coverity with the modified assertions, CID-1524417
disappeared while the report for CID-1524423 no longer claimed that the
assertion tripped.

Coincidentally, it turns out that it is possible to more accurately
annotate our headers than the Coverity modelling file permits in the
case of format strings. Since we can do that and this patch annotates
headers whenever `__coverity_panic__()` would have been used in the
model file, we drop all models that use `__coverity_panic__()` from the
model file.

Upon seeing the success in eliminating false positives involving
assertions, it occurred to me that we could also modify our headers to
eliminate coverity's false positives involving byte swaps. We now have
coverity specific byteswap macros, that do nothing, to disable
Coverity's false positives when we do byte swaps. This allowed us to
also drop the byteswap definitions from the model file.

Lastly, a model file update has been done beyond the mentioned
deletions:

 * The definitions of `umem_alloc_aligned()`, `umem_alloc()` andi
   `umem_zalloc()` were originally implemented in a way that was
   intended to inform coverity that when KM_SLEEP has been passed these
   functions, they do not return NULL. A small error in how this was
   done was found, so we correct it.

 * Definitions for umem_cache_alloc() and umem_cache_free() have been
   added.

In practice, no false positives were avoided by making these changes,
but in the interest of correctness from future coverity builds, we make
them anyway.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Closes #13902

cmd/zed/zed_log.h

@@ -39,6 +39,7 @@ void zed_log_syslog_close(void);
 
 void zed_log_msg(int priority, const char *fmt, ...);
 
+__attribute__((format(printf, 1, 2), __noreturn__))
 void zed_log_die(const char *fmt, ...);
 
 #endif	/* !ZED_LOG_H */

contrib/coverity/model.c

@@ -31,66 +31,6 @@
 
 int condition0, condition1;
 
-void
-abort()
-{
-	__coverity_panic__();
-}
-
-void
-exit(int status)
-{
-	(void) status;
-
-	__coverity_panic__();
-}
-
-void
-_exit(int status)
-{
-	(void) status;
-
-	__coverity_panic__();
-}
-
-void
-zed_log_die(const char *fmt, ...)
-{
-	__coverity_format_string_sink__(fmt);
-	__coverity_panic__();
-}
-
-void
-panic(const char *fmt, ...)
-{
-	__coverity_format_string_sink__(fmt);
-	__coverity_panic__();
-}
-
-void
-vpanic(const char *fmt, va_list adx)
-{
-	(void) adx;
-
-	__coverity_format_string_sink__(fmt);
-	__coverity_panic__();
-}
-
-void
-uu_panic(const char *format, ...)
-{
-	__coverity_format_string_sink__(format);
-	__coverity_panic__();
-}
-
-int
-libspl_assertf(const char *file, const char *func, int line,
-    const char *format, ...)
-{
-	__coverity_format_string_sink__(format);
-	__coverity_panic__();
-}
-
 int
 ddi_copyin(const void *from, void *to, size_t len, int flags)
 {
@@ -125,7 +65,7 @@ umem_alloc_aligned(size_t size, size_t align, int kmflags)
 {
 	(void) align;
 
-	if (UMEM_NOFAIL & kmflags == UMEM_NOFAIL)
+	if ((UMEM_NOFAIL & kmflags) == UMEM_NOFAIL)
 		return (__coverity_alloc__(size));
 	else if (condition0)
 		return (__coverity_alloc__(size));
@@ -136,7 +76,7 @@ umem_alloc_aligned(size_t size, size_t align, int kmflags)
 void *
 umem_alloc(size_t size, int kmflags)
 {
-	if (UMEM_NOFAIL & kmflags == UMEM_NOFAIL)
+	if ((UMEM_NOFAIL & kmflags) == UMEM_NOFAIL)
 		return (__coverity_alloc__(size));
 	else if (condition0)
 		return (__coverity_alloc__(size));
@@ -147,7 +87,7 @@ umem_alloc(size_t size, int kmflags)
 void *
 umem_zalloc(size_t size, int kmflags)
 {
-	if (UMEM_NOFAIL & kmflags == UMEM_NOFAIL)
+	if ((UMEM_NOFAIL & kmflags) == UMEM_NOFAIL)
 		return (__coverity_alloc__(size));
 	else if (condition0)
 		return (__coverity_alloc__(size));
@@ -163,6 +103,32 @@ umem_free(void *buf, size_t size)
 	__coverity_free__(buf);
 }
 
+typedef struct {} umem_cache_t;
+
+void *
+umem_cache_alloc(umem_cache_t *skc, int flags)
+{
+	(void) skc;
+
+	if (condition1)
+		__coverity_sleep__();
+
+	if ((UMEM_NOFAIL & flags) == UMEM_NOFAIL)
+		return (__coverity_alloc_nosize__());
+	else if (condition0)
+		return (__coverity_alloc_nosize__());
+	else
+		return (NULL);
+}
+
+void
+umem_cache_free(umem_cache_t *skc, void *obj)
+{
+	(void) skc;
+
+	__coverity_free__(obj);
+}
+
 void *
 spl_kmem_alloc(size_t sz, int fl, const char *func, int line)
 {
@@ -243,13 +209,6 @@ free(void *buf)
 	__coverity_free__(buf);
 }
 
-int
-spl_panic(const char *file, const char *func, int line, const char *fmt, ...)
-{
-	__coverity_format_string_sink__(fmt);
-	__coverity_panic__();
-}
-
 int
 sched_yield(void)
 {
@@ -398,25 +357,3 @@ __cond_resched(void)
 		__coverity_sleep__();
 	}
 }
-
-/*
- * An endian-independent filesystem must support doing byte swaps on data. We
- * attempt to suppress taint warnings, which are false positives for us.
- */
-void
-byteswap_uint64_array(void *vbuf, size_t size)
-{
-	__coverity_tainted_data_sanitize__(vbuf);
-}
-
-void
-byteswap_uint32_array(void *vbuf, size_t size)
-{
-	__coverity_tainted_data_sanitize__(vbuf);
-}
-
-void
-byteswap_uint16_array(void *vbuf, size_t size)
-{
-	__coverity_tainted_data_sanitize__(vbuf);
-}

include/libuutil_impl.h

@@ -42,7 +42,8 @@ extern "C" {
 void uu_set_error(uint_t);
 
 
-void uu_panic(const char *format, ...) __attribute__((format(printf, 1, 2)));
+__attribute__((format(printf, 1, 2), __noreturn__))
+void uu_panic(const char *format, ...);
 
 
 /*

include/os/freebsd/spl/sys/byteorder.h

@@ -44,6 +44,18 @@
 
 #include <sys/endian.h>
 
+#ifdef __COVERITY__
+/*
+ * Coverity's taint warnings from byteswapping are false positives for us.
+ * Suppress them by hiding byteswapping from Coverity.
+ */
+#define	BSWAP_8(x)	((x) & 0xff)
+#define	BSWAP_16(x)	((x) & 0xffff)
+#define	BSWAP_32(x)	((x) & 0xffffffff)
+#define	BSWAP_64(x)	(x)
+
+#else /* __COVERITY__ */
+
 /*
  * Macros to reverse byte order
  */
@@ -52,6 +64,8 @@
 #define	BSWAP_32(x)	((BSWAP_16(x) << 16) | BSWAP_16((x) >> 16))
 #define	BSWAP_64(x)	((BSWAP_32(x) << 32) | BSWAP_32((x) >> 32))
 
+#endif /* __COVERITY__ */
+
 #define	BMASK_8(x)	((x) & 0xff)
 #define	BMASK_16(x)	((x) & 0xffff)
 #define	BMASK_32(x)	((x) & 0xffffffff)

include/os/freebsd/spl/sys/cmn_err.h

@@ -71,7 +71,7 @@ extern void vuprintf(const char *, __va_list)
     __attribute__((format(printf, 1, 0)));
 
 extern void panic(const char *, ...)
-    __attribute__((format(printf, 1, 2)));
+    __attribute__((format(printf, 1, 2), __noreturn__));
 
 #endif /* !_ASM */
 

include/os/freebsd/spl/sys/debug.h

@@ -54,9 +54,16 @@
 /*
  * Common DEBUG functionality.
  */
-int spl_panic(const char *file, const char *func, int line,
+extern void spl_panic(const char *file, const char *func, int line,
-    const char *fmt, ...);
+    const char *fmt, ...) __attribute__((__noreturn__));
-void spl_dumpstack(void);
+extern void spl_dumpstack(void);
+
+static inline int
+spl_assert(const char *buf, const char *file, const char *func, int line)
+{
+	spl_panic(file, func, line, "%s", buf);
+	return (0);
+}
 
 #ifndef expect
 #define	expect(expr, value) (__builtin_expect((expr), (value)))
@@ -69,8 +76,8 @@ void spl_dumpstack(void);
 
 #define	VERIFY(cond)							\
 	(void) (unlikely(!(cond)) &&					\
-	    spl_panic(__FILE__, __FUNCTION__, __LINE__,			\
+	    spl_assert("VERIFY(" #cond ") failed\n",			\
-	    "%s", "VERIFY(" #cond ") failed\n"))
+	    __FILE__, __FUNCTION__, __LINE__))
 
 #define	VERIFY3B(LEFT, OP, RIGHT)	do {				\
 		const boolean_t _verify3_left = (boolean_t)(LEFT);	\
@@ -158,13 +165,14 @@ void spl_dumpstack(void);
 #define	ASSERT0		VERIFY0
 #define	ASSERT		VERIFY
 #define	IMPLY(A, B) \
-	((void)(likely((!(A)) || (B)) || \
+	((void)(likely((!(A)) || (B)) ||				\
-	    spl_panic(__FILE__, __FUNCTION__, __LINE__, \
+	    spl_assert("(" #A ") implies (" #B ")",			\
-	    "(" #A ") implies (" #B ")")))
+	    __FILE__, __FUNCTION__, __LINE__)))
 #define	EQUIV(A, B) \
-	((void)(likely(!!(A) == !!(B)) || \
+	((void)(likely(!!(A) == !!(B)) || 				\
-	    spl_panic(__FILE__, __FUNCTION__, __LINE__, \
+	    spl_assert("(" #A ") is equivalent to (" #B ")",		\
-	    "(" #A ") is equivalent to (" #B ")")))
+	    __FILE__, __FUNCTION__, __LINE__)))
+
 
 #endif /* NDEBUG */
 

include/os/freebsd/spl/sys/kmem.h

@@ -52,8 +52,10 @@ MALLOC_DECLARE(M_SOLARIS);
 
 typedef struct vmem vmem_t;
 
-extern char	*kmem_asprintf(const char *, ...);
+extern char	*kmem_asprintf(const char *, ...)
-extern char *kmem_vasprintf(const char *fmt, va_list ap);
+    __attribute__((format(printf, 1, 2)));
+extern char *kmem_vasprintf(const char *fmt, va_list ap)
+    __attribute__((format(printf, 1, 0)));
 
 typedef struct kmem_cache {
 	char		kc_name[32];
@@ -70,6 +72,7 @@ typedef struct kmem_cache {
 extern uint64_t spl_kmem_cache_inuse(kmem_cache_t *cache);
 extern uint64_t spl_kmem_cache_entry_size(kmem_cache_t *cache);
 
+__attribute__((alloc_size(1)))
 void *zfs_kmem_alloc(size_t size, int kmflags);
 void zfs_kmem_free(void *buf, size_t size);
 uint64_t kmem_size(void);

include/os/linux/spl/sys/byteorder.h

@@ -36,11 +36,26 @@
 
 #include <sys/isa_defs.h>
 
+#ifdef __COVERITY__
+/*
+ * Coverity's taint warnings from byteswapping are false positives for us.
+ * Suppress them by hiding byteswapping from Coverity.
+ */
+
+#define	BSWAP_8(x)	((x) & 0xff)
+#define	BSWAP_16(x)	((x) & 0xffff)
+#define	BSWAP_32(x)	((x) & 0xffffffff)
+#define	BSWAP_64(x)	(x)
+
+#else /* __COVERITY__ */
+
 #define	BSWAP_8(x)	((x) & 0xff)
 #define	BSWAP_16(x)	((BSWAP_8(x) << 8) | BSWAP_8((x) >> 8))
 #define	BSWAP_32(x)	((BSWAP_16(x) << 16) | BSWAP_16((x) >> 16))
 #define	BSWAP_64(x)	((BSWAP_32(x) << 32) | BSWAP_32((x) >> 32))
 
+#endif /* __COVERITY__ */
+
 #define	LE_16(x)	cpu_to_le16(x)
 #define	LE_32(x)	cpu_to_le32(x)
 #define	LE_64(x)	cpu_to_le64(x)

include/os/linux/spl/sys/cmn_err.h

@@ -41,7 +41,7 @@ extern void cmn_err(int, const char *, ...)
 extern void vcmn_err(int, const char *, va_list)
     __attribute__((format(printf, 2, 0)));
 extern void vpanic(const char *, va_list)
-    __attribute__((format(printf, 1, 0)));
+    __attribute__((format(printf, 1, 0), __noreturn__));
 
 #define	fm_panic	panic
 

include/os/linux/spl/sys/debug.h

@@ -54,17 +54,24 @@
 #define	__maybe_unused __attribute__((unused))
 #endif
 
-int spl_panic(const char *file, const char *func, int line,
+extern void spl_panic(const char *file, const char *func, int line,
-    const char *fmt, ...);
+    const char *fmt, ...) __attribute__((__noreturn__));
-void spl_dumpstack(void);
+extern void spl_dumpstack(void);
+
+static inline int
+spl_assert(const char *buf, const char *file, const char *func, int line)
+{
+	spl_panic(file, func, line, "%s", buf);
+	return (0);
+}
 
 #define	PANIC(fmt, a...)						\
 	spl_panic(__FILE__, __FUNCTION__, __LINE__, fmt, ## a)
 
 #define	VERIFY(cond)							\
 	(void) (unlikely(!(cond)) &&					\
-	    spl_panic(__FILE__, __FUNCTION__, __LINE__,			\
+	    spl_assert("VERIFY(" #cond ") failed\n",			\
-	    "%s", "VERIFY(" #cond ") failed\n"))
+	    __FILE__, __FUNCTION__, __LINE__))
 
 #define	VERIFY3B(LEFT, OP, RIGHT)	do {				\
 		const boolean_t _verify3_left = (boolean_t)(LEFT);	\
@@ -152,13 +159,13 @@ void spl_dumpstack(void);
 #define	ASSERT0		VERIFY0
 #define	ASSERT		VERIFY
 #define	IMPLY(A, B) \
-	((void)(likely((!(A)) || (B)) || \
+	((void)(likely((!(A)) || (B)) ||				\
-	    spl_panic(__FILE__, __FUNCTION__, __LINE__, \
+	    spl_assert("(" #A ") implies (" #B ")",			\
-	    "(" #A ") implies (" #B ")")))
+	    __FILE__, __FUNCTION__, __LINE__)))
 #define	EQUIV(A, B) \
-	((void)(likely(!!(A) == !!(B)) || \
+	((void)(likely(!!(A) == !!(B)) || 				\
-	    spl_panic(__FILE__, __FUNCTION__, __LINE__, \
+	    spl_assert("(" #A ") is equivalent to (" #B ")",		\
-	    "(" #A ") is equivalent to (" #B ")")))
+	    __FILE__, __FUNCTION__, __LINE__)))
 
 #endif /* NDEBUG */
 

include/os/linux/spl/sys/kmem.h

@@ -31,8 +31,10 @@
 #include <linux/vmalloc.h>
 
 extern int kmem_debugging(void);
-extern char *kmem_vasprintf(const char *fmt, va_list ap);
+extern char *kmem_vasprintf(const char *fmt, va_list ap)
-extern char *kmem_asprintf(const char *fmt, ...);
+    __attribute__((format(printf, 1, 0)));
+extern char *kmem_asprintf(const char *fmt, ...)
+    __attribute__((format(printf, 1, 2)));
 extern char *kmem_strdup(const char *str);
 extern void kmem_strfree(char *str);
 
@@ -179,8 +181,10 @@ extern unsigned int spl_kmem_alloc_max;
 #define	kmem_free(ptr, sz)	spl_kmem_free((ptr), (sz))
 #define	kmem_cache_reap_active	spl_kmem_cache_reap_active
 
-extern void *spl_kmem_alloc(size_t sz, int fl, const char *func, int line);
+extern void *spl_kmem_alloc(size_t sz, int fl, const char *func, int line)
-extern void *spl_kmem_zalloc(size_t sz, int fl, const char *func, int line);
+    __attribute__((alloc_size(1)));
+extern void *spl_kmem_zalloc(size_t sz, int fl, const char *func, int line)
+    __attribute__((alloc_size(1)));
 extern void spl_kmem_free(const void *ptr, size_t sz);
 
 /*

include/sys/zfs_context.h

@@ -151,10 +151,14 @@ extern "C" {
 
 extern void dprintf_setup(int *argc, char **argv);
 
-extern void cmn_err(int, const char *, ...);
+extern void cmn_err(int, const char *, ...)
-extern void vcmn_err(int, const char *, va_list);
+    __attribute__((format(printf, 2, 3)));
-extern __attribute__((noreturn)) void panic(const char *, ...);
+extern void vcmn_err(int, const char *, va_list)
-extern __attribute__((noreturn)) void vpanic(const char *, va_list);
+    __attribute__((format(printf, 2, 0)));
+extern void panic(const char *, ...)
+    __attribute__((format(printf, 1, 2), noreturn));
+extern void vpanic(const char *, va_list)
+    __attribute__((format(printf, 1, 0), noreturn));
 
 #define	fm_panic	panic
 

lib/libspl/assert.c

@@ -45,8 +45,11 @@ libspl_assertf(const char *file, const char *func, int line,
 	fprintf(stderr, "\n");
 	fprintf(stderr, "ASSERT at %s:%d:%s()", file, line, func);
 	va_end(args);
+
+#if !__has_feature(attribute_analyzer_noreturn) && !defined(__COVERITY__)
 	if (libspl_assert_ok) {
 		return;
 	}
+#endif
 	abort();
 }

lib/libspl/include/assert.h

@@ -34,12 +34,24 @@
 #include <stdarg.h>
 #include <sys/types.h>
 
+/* Workaround for non-Clang compilers */
+#ifndef __has_feature
+#define	__has_feature(x) 0
+#endif
+
+/* We need to workaround libspl_set_assert_ok() that we have for zdb */
+#if __has_feature(attribute_analyzer_noreturn) || defined(__COVERITY__)
+#define	NORETURN	__attribute__((__noreturn__))
+#else
+#define	NORETURN
+#endif
+
 /* Set to non-zero to avoid abort()ing on an assertion failure */
 extern void libspl_set_assert_ok(boolean_t val);
 
 /* printf version of libspl_assert */
 extern void libspl_assertf(const char *file, const char *func, int line,
-    const char *format, ...);
+    const char *format, ...) NORETURN __attribute__((format(printf, 4, 5)));
 
 static inline int
 libspl_assert(const char *buf, const char *file, const char *func, int line)

lib/libspl/include/os/freebsd/sys/byteorder.h

@@ -59,6 +59,18 @@ extern "C" {
  */
 #if !defined(_XPG4_2) || defined(__EXTENSIONS__)
 
+#ifdef __COVERITY__
+/*
+ * Coverity's taint warnings from byteswapping are false positives for us.
+ * Suppress them by hiding byteswapping from Coverity.
+ */
+#define	BSWAP_8(x)	((x) & 0xff)
+#define	BSWAP_16(x)	((x) & 0xffff)
+#define	BSWAP_32(x)	((x) & 0xffffffff)
+#define	BSWAP_64(x)	(x)
+
+#else /* __COVERITY__ */
+
 /*
  * Macros to reverse byte order
  */
@@ -67,6 +79,8 @@ extern "C" {
 #define	BSWAP_32(x)	((BSWAP_16(x) << 16) | BSWAP_16((x) >> 16))
 #define	BSWAP_64(x)	((BSWAP_32(x) << 32) | BSWAP_32((x) >> 32))
 
+#endif /* __COVERITY__ */
+
 #define	BMASK_8(x)	((x) & 0xff)
 #define	BMASK_16(x)	((x) & 0xffff)
 #define	BMASK_32(x)	((x) & 0xffffffff)

lib/libspl/include/os/linux/sys/byteorder.h

@@ -90,6 +90,18 @@ extern	in_port_t ntohs(in_port_t);
 
 #if !defined(_XPG4_2) || defined(__EXTENSIONS__)
 
+#ifdef __COVERITY__
+/*
+ * Coverity's taint warnings from byteswapping are false positives for us.
+ * Suppress them by hiding byteswapping from Coverity.
+ */
+#define	BSWAP_8(x)	((x) & 0xff)
+#define	BSWAP_16(x)	((x) & 0xffff)
+#define	BSWAP_32(x)	((x) & 0xffffffff)
+#define	BSWAP_64(x)	(x)
+
+#else /* __COVERITY__ */
+
 /*
  * Macros to reverse byte order
  */
@@ -98,6 +110,8 @@ extern	in_port_t ntohs(in_port_t);
 #define	BSWAP_32(x)	((BSWAP_16(x) << 16) | BSWAP_16((x) >> 16))
 #define	BSWAP_64(x)	((BSWAP_32(x) << 32) | BSWAP_32((x) >> 32))
 
+#endif /* __COVERITY__ */
+
 #define	BMASK_8(x)	((x) & 0xff)
 #define	BMASK_16(x)	((x) & 0xffff)
 #define	BMASK_32(x)	((x) & 0xffffffff)

lib/libspl/include/umem.h

@@ -83,6 +83,7 @@ const char *_umem_debug_init(void);
 const char *_umem_options_init(void);
 const char *_umem_logging_init(void);
 
+__attribute__((alloc_size(1)))
 static inline void *
 umem_alloc(size_t size, int flags)
 {
@@ -95,6 +96,7 @@ umem_alloc(size_t size, int flags)
 	return (ptr);
 }
 
+__attribute__((alloc_size(1)))
 static inline void *
 umem_alloc_aligned(size_t size, size_t align, int flags)
 {
@@ -116,6 +118,7 @@ umem_alloc_aligned(size_t size, size_t align, int flags)
 	return (ptr);
 }
 
+__attribute__((alloc_size(1)))
 static inline void *
 umem_zalloc(size_t size, int flags)
 {

module/os/freebsd/spl/spl_misc.c

@@ -94,7 +94,7 @@ ddi_copyout(const void *from, void *to, size_t len, int flags)
 	return (copyout(from, to, len));
 }
 
-int
+void
 spl_panic(const char *file, const char *func, int line, const char *fmt, ...)
 {
 	va_list ap;

module/os/linux/spl/spl-err.c

@@ -45,7 +45,7 @@ spl_dumpstack(void)
 }
 EXPORT_SYMBOL(spl_dumpstack);
 
-int
+void
 spl_panic(const char *file, const char *func, int line, const char *fmt, ...)
 {
 	const char *newfile;
@@ -75,7 +75,6 @@ spl_panic(const char *file, const char *func, int line, const char *fmt, ...)
 		schedule();
 
 	/* Unreachable */
-	return (1);
 }
 EXPORT_SYMBOL(spl_panic);