SELinux Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] libsepol, libsemanage: add a macro to silence static analyzer warnings in tests
@ 2019-09-21 21:30 Nicolas Iooss
  2019-09-24 17:10 ` Stephen Smalley
  0 siblings, 1 reply; 2+ messages in thread
From: Nicolas Iooss @ 2019-09-21 21:30 UTC (permalink / raw)
  To: selinux

Several static analyzers (clang's one, Facebook Infer, etc.) warn about
NULL pointer dereferences after a call to CU_ASSERT_PTR_NOT_NULL_FATAL()
in the test code written using CUnit framework. This is because this
CUnit macro is too complex for them to understand that the pointer
cannot be NULL: it is translated to a call to CU_assertImplementation()
with an argument as TRUE in order to mean that the call is fatal if the
asserted condition failed (cf.
http://cunit.sourceforge.net/doxdocs/group__Framework.html).

A possible solution could consist in replacing the
CU_ASSERT_..._FATAL() calls by assert() ones, as most static analyzers
know about assert(). Nevertheless this seems to go against CUnit's API.

An alternative solution consists in overriding CU_ASSERT_..._FATAL()
macros in order to expand to assert() after a call to the matching
CU_ASSERT_...() non-fatal macro. This appears to work fine and to remove
many false-positive warnings from various static analyzers.

As this substitution should only occur when using static analyzer, put
it under #ifdef __CHECKER__, which is the macro used by sparse when
analyzing the Linux kernel.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsemanage/tests/test_utilities.c      |  2 ++
 libsemanage/tests/utilities.h           | 24 ++++++++++++++++++++++
 libsepol/tests/helpers.h                | 27 +++++++++++++++++++++++++
 libsepol/tests/test-common.c            |  2 ++
 libsepol/tests/test-deps.c              |  2 ++
 libsepol/tests/test-expander-attr-map.c |  1 +
 libsepol/tests/test-expander-roles.c    |  1 +
 libsepol/tests/test-expander-users.c    |  1 +
 scripts/run-scan-build                  |  6 +++++-
 9 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/libsemanage/tests/test_utilities.c b/libsemanage/tests/test_utilities.c
index 601508c2c386..ba995b5ae094 100644
--- a/libsemanage/tests/test_utilities.c
+++ b/libsemanage/tests/test_utilities.c
@@ -34,6 +34,8 @@
 #include <string.h>
 #include <unistd.h>
 
+#include "utilities.h"
+
 void test_semanage_is_prefix(void);
 void test_semanage_split_on_space(void);
 void test_semanage_split(void);
diff --git a/libsemanage/tests/utilities.h b/libsemanage/tests/utilities.h
index c9d54d1e1b76..1d15a98d9db1 100644
--- a/libsemanage/tests/utilities.h
+++ b/libsemanage/tests/utilities.h
@@ -41,6 +41,30 @@
 		CU_ASSERT_STRING_EQUAL(__str, __str2); \
 	} while (0)
 
+
+/* Override CU_*_FATAL() in order to help static analyzers by really asserting that an assertion holds */
+#ifdef __CHECKER__
+
+#undef CU_ASSERT_FATAL
+#define CU_ASSERT_FATAL(value) do { \
+		CU_ASSERT(value); \
+		assert(value); \
+	} while (0)
+
+#undef CU_FAIL_FATAL
+#define CU_FAIL_FATAL(msg) do { \
+		CU_FAIL(msg); \
+		assert(0); \
+	} while (0)
+
+#undef CU_ASSERT_PTR_NOT_NULL_FATAL
+#define CU_ASSERT_PTR_NOT_NULL_FATAL(value) do { \
+		CU_ASSERT_PTR_NOT_NULL(value); \
+		assert((value) != NULL); \
+	} while (0)
+
+#endif /* __CHECKER__ */
+
 #define I_NULL  -1
 #define I_FIRST  0
 #define I_SECOND 1
diff --git a/libsepol/tests/helpers.h b/libsepol/tests/helpers.h
index 10d390946499..34b8f21e10e0 100644
--- a/libsepol/tests/helpers.h
+++ b/libsepol/tests/helpers.h
@@ -24,9 +24,36 @@
 
 #include <sepol/policydb/policydb.h>
 #include <sepol/policydb/conditional.h>
+#include <CUnit/Basic.h>
 
 /* helper functions */
 
+/* Override CU_*_FATAL() in order to help static analyzers by really asserting that an assertion holds */
+#ifdef __CHECKER__
+
+#include <assert.h>
+
+#undef CU_ASSERT_FATAL
+#define CU_ASSERT_FATAL(value) do { \
+		CU_ASSERT(value); \
+		assert(value); \
+	} while (0)
+
+#undef CU_FAIL_FATAL
+#define CU_FAIL_FATAL(msg) do { \
+		CU_FAIL(msg); \
+		assert(0); \
+	} while (0)
+
+#undef CU_ASSERT_PTR_NOT_NULL_FATAL
+#define CU_ASSERT_PTR_NOT_NULL_FATAL(value) do { \
+		CU_ASSERT_PTR_NOT_NULL(value); \
+		assert((value) != NULL); \
+	} while (0)
+
+#endif /* __CHECKER__ */
+
+
 /* Load a source policy into p. policydb_init will called within this function.
  * 
  * Example: test_load_policy(p, POLICY_BASE, 1, "foo", "base.conf") will load the
diff --git a/libsepol/tests/test-common.c b/libsepol/tests/test-common.c
index 1d902880fe2e..f690635eee27 100644
--- a/libsepol/tests/test-common.c
+++ b/libsepol/tests/test-common.c
@@ -26,6 +26,8 @@
 
 #include <CUnit/Basic.h>
 
+#include "helpers.h"
+
 void test_sym_presence(policydb_t * p, const char *id, int sym_type, unsigned int scope_type, unsigned int *decls, unsigned int len)
 {
 	scope_datum_t *scope;
diff --git a/libsepol/tests/test-deps.c b/libsepol/tests/test-deps.c
index 6bbba810564a..f4ab09ba0dbf 100644
--- a/libsepol/tests/test-deps.c
+++ b/libsepol/tests/test-deps.c
@@ -66,6 +66,8 @@
 #include <sepol/debug.h>
 #include <sepol/handle.h>
 
+#include "helpers.h"
+
 #define BASE_MODREQ_TYPE_GLOBAL    0
 #define BASE_MODREQ_ATTR_GLOBAL    1
 #define BASE_MODREQ_OBJ_GLOBAL     2
diff --git a/libsepol/tests/test-expander-attr-map.c b/libsepol/tests/test-expander-attr-map.c
index d10636ca09a8..a9744541fe00 100644
--- a/libsepol/tests/test-expander-attr-map.c
+++ b/libsepol/tests/test-expander-attr-map.c
@@ -21,6 +21,7 @@
 
 #include "test-expander-attr-map.h"
 #include "test-common.h"
+#include "helpers.h"
 
 #include <sepol/policydb/policydb.h>
 #include <CUnit/Basic.h>
diff --git a/libsepol/tests/test-expander-roles.c b/libsepol/tests/test-expander-roles.c
index aba3c9bd19c4..74c781b85e68 100644
--- a/libsepol/tests/test-expander-roles.c
+++ b/libsepol/tests/test-expander-roles.c
@@ -22,6 +22,7 @@
 
 #include "test-expander-roles.h"
 #include "test-common.h"
+#include "helpers.h"
 
 #include <sepol/policydb/policydb.h>
 #include <CUnit/Basic.h>
diff --git a/libsepol/tests/test-expander-users.c b/libsepol/tests/test-expander-users.c
index 9d9c7a62f215..ab2265c110d5 100644
--- a/libsepol/tests/test-expander-users.c
+++ b/libsepol/tests/test-expander-users.c
@@ -21,6 +21,7 @@
  */
 
 #include "test-expander-users.h"
+#include "helpers.h"
 
 #include <sepol/policydb/policydb.h>
 #include <CUnit/Basic.h>
diff --git a/scripts/run-scan-build b/scripts/run-scan-build
index 88fe551ce698..ae5aa48b4a05 100755
--- a/scripts/run-scan-build
+++ b/scripts/run-scan-build
@@ -22,7 +22,11 @@ export RUBYLIB="$DESTDIR/$(${RUBY:-ruby} -e 'puts RbConfig::CONFIG["vendorlibdir
 
 # Build and analyze
 make -C .. CC=clang clean distclean -j"$(nproc)"
-scan-build -analyze-headers -o "$OUTPUTDIR" make -C .. CC=clang DESTDIR="$DESTDIR" install install-pywrap install-rubywrap all test
+scan-build -analyze-headers -o "$OUTPUTDIR" make -C .. \
+    CC=clang \
+    DESTDIR="$DESTDIR" \
+    CFLAGS="-O2 -Wall -D__CHECKER__ -I$DESTDIR/usr/include" \
+    install install-pywrap install-rubywrap all test
 
 # Reduce the verbosity in order to keep the message from scan-build saying
 # "scan-build: Run 'scan-view /.../output-scan-build/2018-...' to examine bug reports.
-- 
2.22.0


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] libsepol, libsemanage: add a macro to silence static analyzer warnings in tests
  2019-09-21 21:30 [PATCH] libsepol, libsemanage: add a macro to silence static analyzer warnings in tests Nicolas Iooss
@ 2019-09-24 17:10 ` Stephen Smalley
  0 siblings, 0 replies; 2+ messages in thread
From: Stephen Smalley @ 2019-09-24 17:10 UTC (permalink / raw)
  To: Nicolas Iooss, selinux

On 9/21/19 5:30 PM, Nicolas Iooss wrote:
> Several static analyzers (clang's one, Facebook Infer, etc.) warn about
> NULL pointer dereferences after a call to CU_ASSERT_PTR_NOT_NULL_FATAL()
> in the test code written using CUnit framework. This is because this
> CUnit macro is too complex for them to understand that the pointer
> cannot be NULL: it is translated to a call to CU_assertImplementation()
> with an argument as TRUE in order to mean that the call is fatal if the
> asserted condition failed (cf.
> http://cunit.sourceforge.net/doxdocs/group__Framework.html).
> 
> A possible solution could consist in replacing the
> CU_ASSERT_..._FATAL() calls by assert() ones, as most static analyzers
> know about assert(). Nevertheless this seems to go against CUnit's API.
> 
> An alternative solution consists in overriding CU_ASSERT_..._FATAL()
> macros in order to expand to assert() after a call to the matching
> CU_ASSERT_...() non-fatal macro. This appears to work fine and to remove
> many false-positive warnings from various static analyzers.
> 
> As this substitution should only occur when using static analyzer, put
> it under #ifdef __CHECKER__, which is the macro used by sparse when
> analyzing the Linux kernel.
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>   libsemanage/tests/test_utilities.c      |  2 ++
>   libsemanage/tests/utilities.h           | 24 ++++++++++++++++++++++
>   libsepol/tests/helpers.h                | 27 +++++++++++++++++++++++++
>   libsepol/tests/test-common.c            |  2 ++
>   libsepol/tests/test-deps.c              |  2 ++
>   libsepol/tests/test-expander-attr-map.c |  1 +
>   libsepol/tests/test-expander-roles.c    |  1 +
>   libsepol/tests/test-expander-users.c    |  1 +
>   scripts/run-scan-build                  |  6 +++++-
>   9 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/libsemanage/tests/test_utilities.c b/libsemanage/tests/test_utilities.c
> index 601508c2c386..ba995b5ae094 100644
> --- a/libsemanage/tests/test_utilities.c
> +++ b/libsemanage/tests/test_utilities.c
> @@ -34,6 +34,8 @@
>   #include <string.h>
>   #include <unistd.h>
>   
> +#include "utilities.h"
> +
>   void test_semanage_is_prefix(void);
>   void test_semanage_split_on_space(void);
>   void test_semanage_split(void);
> diff --git a/libsemanage/tests/utilities.h b/libsemanage/tests/utilities.h
> index c9d54d1e1b76..1d15a98d9db1 100644
> --- a/libsemanage/tests/utilities.h
> +++ b/libsemanage/tests/utilities.h
> @@ -41,6 +41,30 @@
>   		CU_ASSERT_STRING_EQUAL(__str, __str2); \
>   	} while (0)
>   
> +
> +/* Override CU_*_FATAL() in order to help static analyzers by really asserting that an assertion holds */
> +#ifdef __CHECKER__
> +
> +#undef CU_ASSERT_FATAL
> +#define CU_ASSERT_FATAL(value) do { \
> +		CU_ASSERT(value); \
> +		assert(value); \
> +	} while (0)

You only want to evaluate value once and save the result for use in both 
CU_ASSERT() and assert() or it could change, e.g.
         CU_ASSERT_FATAL(semanage_ibendport_del_local(sh, key) >= 0);

> +
> +#undef CU_FAIL_FATAL
> +#define CU_FAIL_FATAL(msg) do { \
> +		CU_FAIL(msg); \
> +		assert(0); \
> +	} while (0)
> +
> +#undef CU_ASSERT_PTR_NOT_NULL_FATAL
> +#define CU_ASSERT_PTR_NOT_NULL_FATAL(value) do { \
> +		CU_ASSERT_PTR_NOT_NULL(value); \
> +		assert((value) != NULL); \
> +	} while (0)

Ditto.

> +
> +#endif /* __CHECKER__ */
> +
>   #define I_NULL  -1
>   #define I_FIRST  0
>   #define I_SECOND 1
> diff --git a/libsepol/tests/helpers.h b/libsepol/tests/helpers.h
> index 10d390946499..34b8f21e10e0 100644
> --- a/libsepol/tests/helpers.h
> +++ b/libsepol/tests/helpers.h
> @@ -24,9 +24,36 @@
>   
>   #include <sepol/policydb/policydb.h>
>   #include <sepol/policydb/conditional.h>
> +#include <CUnit/Basic.h>
>   
>   /* helper functions */
>   
> +/* Override CU_*_FATAL() in order to help static analyzers by really asserting that an assertion holds */
> +#ifdef __CHECKER__
> +
> +#include <assert.h>
> +
> +#undef CU_ASSERT_FATAL
> +#define CU_ASSERT_FATAL(value) do { \
> +		CU_ASSERT(value); \
> +		assert(value); \
> +	} while (0)
> +
> +#undef CU_FAIL_FATAL
> +#define CU_FAIL_FATAL(msg) do { \
> +		CU_FAIL(msg); \
> +		assert(0); \
> +	} while (0)
> +
> +#undef CU_ASSERT_PTR_NOT_NULL_FATAL
> +#define CU_ASSERT_PTR_NOT_NULL_FATAL(value) do { \
> +		CU_ASSERT_PTR_NOT_NULL(value); \
> +		assert((value) != NULL); \
> +	} while (0)
> +
> +#endif /* __CHECKER__ */
> +
> +
>   /* Load a source policy into p. policydb_init will called within this function.
>    *
>    * Example: test_load_policy(p, POLICY_BASE, 1, "foo", "base.conf") will load the
> diff --git a/libsepol/tests/test-common.c b/libsepol/tests/test-common.c
> index 1d902880fe2e..f690635eee27 100644
> --- a/libsepol/tests/test-common.c
> +++ b/libsepol/tests/test-common.c
> @@ -26,6 +26,8 @@
>   
>   #include <CUnit/Basic.h>
>   
> +#include "helpers.h"
> +
>   void test_sym_presence(policydb_t * p, const char *id, int sym_type, unsigned int scope_type, unsigned int *decls, unsigned int len)
>   {
>   	scope_datum_t *scope;
> diff --git a/libsepol/tests/test-deps.c b/libsepol/tests/test-deps.c
> index 6bbba810564a..f4ab09ba0dbf 100644
> --- a/libsepol/tests/test-deps.c
> +++ b/libsepol/tests/test-deps.c
> @@ -66,6 +66,8 @@
>   #include <sepol/debug.h>
>   #include <sepol/handle.h>
>   
> +#include "helpers.h"
> +
>   #define BASE_MODREQ_TYPE_GLOBAL    0
>   #define BASE_MODREQ_ATTR_GLOBAL    1
>   #define BASE_MODREQ_OBJ_GLOBAL     2
> diff --git a/libsepol/tests/test-expander-attr-map.c b/libsepol/tests/test-expander-attr-map.c
> index d10636ca09a8..a9744541fe00 100644
> --- a/libsepol/tests/test-expander-attr-map.c
> +++ b/libsepol/tests/test-expander-attr-map.c
> @@ -21,6 +21,7 @@
>   
>   #include "test-expander-attr-map.h"
>   #include "test-common.h"
> +#include "helpers.h"
>   
>   #include <sepol/policydb/policydb.h>
>   #include <CUnit/Basic.h>
> diff --git a/libsepol/tests/test-expander-roles.c b/libsepol/tests/test-expander-roles.c
> index aba3c9bd19c4..74c781b85e68 100644
> --- a/libsepol/tests/test-expander-roles.c
> +++ b/libsepol/tests/test-expander-roles.c
> @@ -22,6 +22,7 @@
>   
>   #include "test-expander-roles.h"
>   #include "test-common.h"
> +#include "helpers.h"
>   
>   #include <sepol/policydb/policydb.h>
>   #include <CUnit/Basic.h>
> diff --git a/libsepol/tests/test-expander-users.c b/libsepol/tests/test-expander-users.c
> index 9d9c7a62f215..ab2265c110d5 100644
> --- a/libsepol/tests/test-expander-users.c
> +++ b/libsepol/tests/test-expander-users.c
> @@ -21,6 +21,7 @@
>    */
>   
>   #include "test-expander-users.h"
> +#include "helpers.h"
>   
>   #include <sepol/policydb/policydb.h>
>   #include <CUnit/Basic.h>
> diff --git a/scripts/run-scan-build b/scripts/run-scan-build
> index 88fe551ce698..ae5aa48b4a05 100755
> --- a/scripts/run-scan-build
> +++ b/scripts/run-scan-build
> @@ -22,7 +22,11 @@ export RUBYLIB="$DESTDIR/$(${RUBY:-ruby} -e 'puts RbConfig::CONFIG["vendorlibdir
>   
>   # Build and analyze
>   make -C .. CC=clang clean distclean -j"$(nproc)"
> -scan-build -analyze-headers -o "$OUTPUTDIR" make -C .. CC=clang DESTDIR="$DESTDIR" install install-pywrap install-rubywrap all test
> +scan-build -analyze-headers -o "$OUTPUTDIR" make -C .. \
> +    CC=clang \
> +    DESTDIR="$DESTDIR" \
> +    CFLAGS="-O2 -Wall -D__CHECKER__ -I$DESTDIR/usr/include" \
> +    install install-pywrap install-rubywrap all test
>   
>   # Reduce the verbosity in order to keep the message from scan-build saying
>   # "scan-build: Run 'scan-view /.../output-scan-build/2018-...' to examine bug reports.
> 


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-21 21:30 [PATCH] libsepol, libsemanage: add a macro to silence static analyzer warnings in tests Nicolas Iooss
2019-09-24 17:10 ` Stephen Smalley

SELinux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \
		selinux@vger.kernel.org selinux@archiver.kernel.org
	public-inbox-index selinux

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.selinux


AGPL code for this site: git clone https://public-inbox.org/ public-inbox