Disable assertions for release builds

Currently our build generates very many warnings if
G_DISABLE_ASSERT is defined.

Add ws_assert() and ws_assert_not_reached() to incrementally
replace existing assertions and then disable them using
WS_DISABLE_ASSERT.

Assertions are disabled with CMake build type Release.
By default the build type is RelWithDebInfo so the current
behaviour of enabling assertions by default is (for now) preserved.

Add some notes to README.Developer.

CMakeLists.txt

@@ -814,6 +814,15 @@ else() # ! MSVC
 		-DG_DISABLE_DEPRECATED
 		-DG_DISABLE_SINGLE_INCLUDES
 	)
+	if(CMAKE_VERSION VERSION_GREATER "3.11.99")
+		# Build type dependency requires support for generator expressions.
+		# add_compile_definitions() requires cmake >= 3.12.
+		add_compile_definitions(
+			$<$<OR:$<BOOL:${DISABLE_ASSERT}>,$<CONFIG:Release>>:WS_DISABLE_ASSERT>
+		)
+	elseif(DISABLE_ASSERT)
+		add_definitions(-DWS_DISABLE_ASSERT)
+	endif()
 
 	set(WIRESHARK_LD_FLAGS
 		# See also CheckCLinkerFlag.cmake

CMakeOptions.txt

@@ -44,6 +44,7 @@ endif()
 option(BUILD_mmdbresolve   "Build MaxMind DB resolver" ON)
 
 option(DISABLE_WERROR    "Do not treat warnings as errors" OFF)
+option(DISABLE_ASSERT    "Disable assertions" OFF)
 option(DISABLE_FRAME_LARGER_THAN_WARNING "Disable warning if the size of a function frame is large" OFF)
 option(EXTCAP_ANDROIDDUMP_LIBPCAP    "Build androiddump using libpcap" OFF)
 option(ENABLE_EXTRA_COMPILER_WARNINGS "Do additional compiler warnings (disables -Werror)" OFF)

doc/README.developer

@@ -533,7 +533,7 @@ packets without crashing or looping infinitely.
 Here are some suggestions for making code more robust in the face
 of incorrectly-formed packets:
 
-Do *NOT* use "g_assert()" or "g_assert_not_reached()" in dissectors.
+Do *NOT* use "ws_assert()" or "ws_assert_not_reached()" with input data in dissectors.
 *NO* value in a packet's data should be considered "wrong" in the sense
 that it's a problem with the dissector if found; if it cannot do
 anything else with a particular value from a packet's data, the
@@ -541,6 +541,35 @@ dissector should put into the protocol tree an indication that the
 value is invalid, and should return.  The "expert" mechanism should be
 used for that purpose.
 
+Use assertions to catch logic errors in your program. A failed assertion
+indicates a bug in the code. Use ws_assert() instead of g_assert() to
+test a logic condition. Note that ws_assert() will be removed with
+WS_DISABLE_ASSERT. Therefore assertions should not have any side-effects,
+otherwise the program may behave inconsistently.
+
+Use ws_assert_not_reached() instead of g_assert_not_reached() for
+unreachable error conditions. For example if (and only if) you know
+'myvar' can only have the values 1 and 2 do:
+    switch(myvar) {
+    case 1:
+        (...)
+        break;
+    case 2:
+        (...)
+        break;
+    default:
+        ws_assert_not_reached();
+        break;
+    }
+
+For dissectors use DISSECTOR_ASSERT() and DISSECTOR_ASSERT_NOT_REACHED()
+instead, with the same caveats as above.
+
+You should continue to use g_assert_true(), g_assert_cmpstr(), etc for
+"test code", such as unit testing. These assertions are always active.
+See the GLib Testing API documentation for the details on each of those
+functions.
+
 If there is a case where you are checking not for an invalid data item
 in the packet, but for a bug in the dissector (for example, an
 assumption being made at a particular point in the code about the

version_info.c

@@ -174,6 +174,11 @@ get_compiled_version_info(void (*prepend_info)(GString *),
 	/* Additional application-dependent information */
 	if (append_info)
 		(*append_info)(str);
+
+#ifdef WS_DISABLE_ASSERT
+	g_string_append(str, ", without assertions");
+#endif
+
 	g_string_append(str, ".");
 
 	end_string(str);

wsutil/CMakeLists.txt

@@ -65,6 +65,7 @@ set(WSUTIL_PUBLIC_HEADERS
 	type_util.h
 	unicode-utils.h
 	utf8_entities.h
+	ws_assert.h
 	ws_cpuid.h
 	glib-compat.h
 	ws_mempbrk.h

wsutil/ws_assert.h

@@ -0,0 +1,45 @@
+/* ws_assert.h
+ *
+ * Wireshark - Network traffic analyzer
+ * By Gerald Combs <gerald@wireshark.org>
+ * Copyright 1998 Gerald Combs
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef __WS_ASSERT_H__
+#define __WS_ASSERT_H__
+
+#include <ws_attributes.h>
+
+/*
+ * ws_assert() cannot produce side effects, otherwise code will
+ * behave differently because of WS_DISABLE_ASSERT, and probably introduce
+ * some nasty bugs.
+ */
+#ifndef WS_DISABLE_ASSERT
+#define ws_assert(expr) g_assert(expr)
+#else
+#define ws_assert(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.
+ *
+ * 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
+ * causes the spurious warnings, because the compiler can't tell anymore
+ * that a certain code path is not used. We add the call to g_abort() so
+ * that the function never returns, even with G_DISABLE_ASSERT.
+ */
+static inline
+WS_NORETURN void ws_assert_not_reached(void) {
+    g_assert_not_reached();
+    g_abort();
+};
+
+#endif /* __WS_ASSERT_H__ */