From 8eacd615c8437bcb058d01d2446f8149ae9fda25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Valverde?= Date: Wed, 3 Mar 2021 04:10:02 +0000 Subject: [PATCH] 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 | 9 +++++++++ CMakeOptions.txt | 1 + doc/README.developer | 31 ++++++++++++++++++++++++++++- version_info.c | 5 +++++ wsutil/CMakeLists.txt | 1 + wsutil/ws_assert.h | 45 +++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 wsutil/ws_assert.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 2e560d53de..856146d439 100644 --- a/CMakeLists.txt +++ b/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( + $<$,$>:WS_DISABLE_ASSERT> + ) + elseif(DISABLE_ASSERT) + add_definitions(-DWS_DISABLE_ASSERT) + endif() set(WIRESHARK_LD_FLAGS # See also CheckCLinkerFlag.cmake diff --git a/CMakeOptions.txt b/CMakeOptions.txt index a7297f361e..799ed2165b 100644 --- a/CMakeOptions.txt +++ b/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) diff --git a/doc/README.developer b/doc/README.developer index c7658962ee..99108c7470 100644 --- a/doc/README.developer +++ b/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 diff --git a/version_info.c b/version_info.c index 8961dbc6e9..49c5d2cfd3 100644 --- a/version_info.c +++ b/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); diff --git a/wsutil/CMakeLists.txt b/wsutil/CMakeLists.txt index d5c74cf3ac..85d94a9b5b 100644 --- a/wsutil/CMakeLists.txt +++ b/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 diff --git a/wsutil/ws_assert.h b/wsutil/ws_assert.h new file mode 100644 index 0000000000..b8c18f619c --- /dev/null +++ b/wsutil/ws_assert.h @@ -0,0 +1,45 @@ +/* ws_assert.h + * + * Wireshark - Network traffic analyzer + * By Gerald Combs + * Copyright 1998 Gerald Combs + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef __WS_ASSERT_H__ +#define __WS_ASSERT_H__ + +#include + +/* + * 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__ */