wsutil: Rewrite ws_assert() to minimize dependencies

This includes as little as possible in the assertion header, so
that it can be included globally in every file without pulling
any unwanted definitions. In particular pulling stdlib.h is
avoided because that can have side effects if it wants to
include non-portable extensions.

It is possible to have side-effects from include glib.h too, for
example because of G_LOG_DOMAIN.

These side-effects are usually avoidable with careful ordering
of pre-processor directives but with multiple levels of indirections
it can be hard to track. Better to make it robust to these kinds
of failures in the first place.

Also integrate with our logger for a cohesive experience (but
keep it a private dependency).

debian/libwsutil0.symbols

@@ -203,6 +203,7 @@ libwsutil.so.0 libwsutil0 #MINVER#
  ws_add_crash_info@Base 1.10.0
  ws_ascii_strnatcasecmp@Base 1.99.1
  ws_ascii_strnatcmp@Base 1.99.1
+ ws_assert_failed@Base 3.5.0
  ws_base32_decode@Base 2.3.0
  ws_basestrtou16@Base 2.9.0
  ws_basestrtou32@Base 2.9.0

epan/CMakeLists.txt

@@ -386,7 +386,7 @@ install(FILES ${LIBWIRESHARK_PUBLIC_HEADERS}
 )
 
 add_executable(exntest EXCLUDE_FROM_ALL exntest.c except.c)
-target_link_libraries(exntest ${GLIB2_LIBRARIES})
+target_link_libraries(exntest epan)
 set_target_properties(exntest PROPERTIES
 	FOLDER "Tests"
 	EXCLUDE_FROM_DEFAULT_BUILD True

wsutil/CMakeLists.txt

@@ -122,6 +122,7 @@ set(WSUTIL_COMMON_FILES
 	type_util.c
 	unicode-utils.c
 	glib-compat.c
+	ws_assert.c
 	ws_mempbrk.c
 	ws_pipe.c
 	wsgcrypt.c

wsutil/ws_assert.c

@@ -0,0 +1,30 @@
+/* ws_assert.c
+ *
+ * Wireshark - Network traffic analyzer
+ * By Gerald Combs <gerald@wireshark.org>
+ * Copyright 1998 Gerald Combs
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "config.h"
+
+#include "ws_assert.h"
+#include <stdlib.h>
+
+#include "wslog.h"
+
+
+void ws_assert_failed(const char *file, int line, const char *function,
+                    const char *domain, const char *assertion,
+                    bool unreachable)
+{
+    if (unreachable)
+        ws_log_full(domain, LOG_LEVEL_ERROR, file, line, function,
+                        "assertion \"not reached\" failed");
+    else
+        ws_log_full(domain, LOG_LEVEL_ERROR, file, line, function,
+                        "assertion failed: %s", assertion);
+
+    abort();
+}

wsutil/ws_assert.h

@@ -10,43 +10,74 @@
 #ifndef __WS_ASSERT_H__
 #define __WS_ASSERT_H__
 
+#include <ws_symbol_export.h>
 #include <ws_attributes.h>
-#include <stdlib.h>
+#include <stdbool.h>
 
+#ifdef WS_LOG_DOMAIN
+#define _ASSERT_DOMAIN WS_LOG_DOMAIN
+#else
+#define _ASSERT_DOMAIN ""
+#endif
+
+#ifdef __cplusplus
+extern "C" {
+#endif /* __cplusplus */
+
+WS_DLL_PUBLIC
+WS_NORETURN
+void ws_assert_failed(const char *file, int line, const char *function,
+                        const char *domain, const char *assertion,
+                        bool unreachable);
+
+#define _ASSERT_FAIL(expr) \
+        ws_assert_failed(__FILE__, __LINE__, __func__, \
+                            _ASSERT_DOMAIN, #expr, false)
+
+/*
+ * ws_abort_if_fail() is not conditional on WS_DISABLE_ASSERT.
+ * Usually used to appease a static analyzer.
+ */
+#define ws_abort_if_fail(expr) \
+        do { if (!(expr)) _ASSERT_FAIL(expr); } while (0)
+
+#ifdef WS_DISABLE_ASSERT
 /*
  * ws_assert() cannot produce side effects, otherwise code will
  * behave differently because of WS_DISABLE_ASSERT, and probably introduce
- * some nasty bugs.
+ * some difficult to track bugs.
+ *
+ * We don't want to execute the expression with WS_DISABLE_ASSERT because
+ * it might be time and space costly and the goal here is to optimize for
+ * WS_DISABLE_ASSERT. However removing it completely is not good enough
+ * because it might generate many unused variable warnings. So we use
+ * if (false) and let the compiler optimize away the dead execution branch.
  */
-#ifndef WS_DISABLE_ASSERT
-#define ws_assert(expr) g_assert(expr)
+#define ws_assert(expr) do { if (false) ws_abort_if_fail(expr); } while (0)
 #else
-#define ws_assert(expr) G_STMT_START { if (0) g_assert(expr); } G_STMT_END
+#define ws_assert(expr) ws_abort_if_fail(expr)
 #endif
 
 /*
  * We don't want to disable ws_assert_not_reached() with WS_DISABLE_ASSERT.
  * That would blast compiler warnings everywhere for no benefit, not
- * even a miniscule performance gain.
+ * even a miniscule performance gain. Reaching this function is always
+ * a programming error and will unconditionally abort execution.
  *
- * Note: If the compiler supports unreachable built-ins (which recent
- * versions of GCC and MSVC do) there is no warning blast with
- * g_assert_not_reached() and G_DISABLE_ASSERT. However if that is not
- * the case then g_assert_not_reached() is simply (void)0 and that
+ * Note: With g_assert_not_reached() if the compiler supports unreachable
+ * built-ins (which recent versions of GCC and MSVC do) there is no warning
+ * blast with g_assert_not_reached() and G_DISABLE_ASSERT. However if that
+ * is not the case then g_assert_not_reached() is simply (void)0 and that
  * causes the spurious warnings, because the compiler can't tell anymore
- * that a certain code path is not used. We add the call to abort() so
- * that the function never returns, even with G_DISABLE_ASSERT.
+ * that a certain code path is not used. We avoid that with
+ * ws_assert_not_reached(). There is no reason to ever use a no-op here.
  */
-static inline
-WS_NORETURN void ws_assert_not_reached(void) {
-    g_assert_not_reached();
-    abort();
-}
+#define ws_assert_not_reached() \
+        ws_assert_failed(__FILE__, __LINE__, __func__, \
+                            _ASSERT_DOMAIN, NULL, true)
 
-/*
- * ws_abort_if_fail() is always enabled. Usually used to appease a static
- * analyzer.
- */
-#define ws_abort_if_fail(expr) g_assert_true(expr)
+#ifdef __cplusplus
+}
+#endif /* __cplusplus */
 
 #endif /* __WS_ASSERT_H__ */