hardening: add insurance to exit on a failed ASSERT()

The code behind our ASSERT() macro is pretty complex. Although it seems to be correct, make it trivially clear we will never return from a failed assert by adding an _exit(1) call. As was suggested by Sebastian Krahmer of the SuSE security team.

A secondary benefit is that tools like clang static analyzer and coverity can now understand our ASSERT() macros too. To make sure they do, change assert_failed() to a static inline function.

---
 src/openvpn/error.c | 6 ------
 src/openvpn/error.h | 7 ++++++-
 2 files changed, 6 insertions(+), 7 deletions(-)

index 77b6cec..3f6254d 100644
@@ -393,12 +393,6 @@ dont_mute (unsigned int flags)
   return ret;
 }
 
-void
-assert_failed (const char *filename, int line)
-{
-  msg (M_FATAL, "Assertion failed at %s:%d", filename, line);
-}
-
 /*
  * Fail memory allocation.  Don't use msg() because it tries
  * to allocate memory as part of its operation.

index d5204f3..6db7b59 100644
@@ -210,7 +210,12 @@ FILE *msg_fp(const unsigned int flags);
 /* Fatal logic errors */
 #define ASSERT(x) do { if (!(x)) assert_failed(__FILE__, __LINE__); } while (false)
 
-void assert_failed (const char *filename, int line);
+static inline void
+assert_failed (const char *filename, int line)
+{
+  msg (M_FATAL, "Assertion failed at %s:%d", filename, line);
+  _exit(1); /* Just insurance; M_FATAL should trigger a clean exit. */
+}
 
 #ifdef ENABLE_DEBUG
 void crash (void); /* force a segfault (debugging only) */

2.1.4

Am 21.10.15 um 00:37 schrieb Steffan Karger:

This change makes absolute sense. ACK from me.

Arne

The code behind our ASSERT() macro is pretty complex. Although it seems to be correct, make it trivially clear we will never return from a failed assert by adding an _exit(1) call. As was suggested by Sebastian Krahmer of the SuSE security team.

To make sure they that tools like clang static analyzer and coverity understand that assert_failed() will not return, add an __attribute__((__noreturn__)) annotation.

v2: use __attribute__ instead of inline to convince static analysers.

---
 src/openvpn/error.c | 1 +
 src/openvpn/error.h | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

index 77b6cec..66f37f3 100644
@@ -397,6 +397,7 @@ void
 assert_failed (const char *filename, int line)
 {
   msg (M_FATAL, "Assertion failed at %s:%d", filename, line);
+  _exit(1);
 }
 
 /*

index d5204f3..4d33843 100644
@@ -210,7 +210,7 @@ FILE *msg_fp(const unsigned int flags);
 /* Fatal logic errors */
 #define ASSERT(x) do { if (!(x)) assert_failed(__FILE__, __LINE__); } while (false)
 
-void assert_failed (const char *filename, int line);
+void assert_failed (const char *filename, int line) __attribute__((__noreturn__));
 
 #ifdef ENABLE_DEBUG
 void crash (void); /* force a segfault (debugging only) */

2.1.4

ACK. Fine, whatever makes the analyzers happy.

Arne

Am 21.10.15 um 10:08 schrieb Steffan Karger:

ACK.

Your patch has been applied to the master and release/2.3 branch.

commit e8a9e3203bf00605dae000d31095076ae038491c (master) commit d03dd06e59dc98eb2afaaa49cb1f879cab9ce747 (release/2.3)

Author: Steffan Karger
Date:   Wed Oct 21 10:08:06 2015 +0200

     hardening: add insurance to exit on a failed ASSERT()

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10349
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering