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__ */