From: David Lamparter Date: Tue, 16 Mar 2021 10:03:44 +0000 (+0100) Subject: lib: rework how we "override" assert() X-Git-Tag: base_8.0~68^2~2 X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=64dd77361f7ab97365d264408afb538097c0339c;p=mirror%2Ffrr.git lib: rework how we "override" assert() The previous method, using zassert.h and hoping nothing includes assert.h (which, on glibc at least, just does "#undef assert" and puts its own definition in...) was fragile - and actually broke undetected. Just provide our own assert.h and control overriding by putting it in a separate directory to add to the include path (or not.) Signed-off-by: David Lamparter --- diff --git a/Makefile.am b/Makefile.am index e4149b62ed..a5101df2f0 100644 --- a/Makefile.am +++ b/Makefile.am @@ -11,11 +11,19 @@ AM_CFLAGS = \ $(SAN_FLAGS) \ $(WERROR) \ # end -AM_CPPFLAGS = \ + +# CPPFLAGS_BASE does not contain the include path for overriding assert.h, +# therefore should be used in tools that do *not* link libfrr or do not want +# assert() overridden +CPPFLAGS_BASE = \ -I$(top_srcdir) -I$(top_srcdir)/include -I$(top_srcdir)/lib \ -I$(top_builddir) \ $(LUA_INCLUDE) \ # end +AM_CPPFLAGS = \ + -I$(top_srcdir)/lib/assert \ + $(CPPFLAGS_BASE) \ + # end AM_LDFLAGS = \ -export-dynamic \ $(AC_LDFLAGS) \ diff --git a/lib/assert/assert.h b/lib/assert/assert.h new file mode 100644 index 0000000000..fbdbd52ce8 --- /dev/null +++ b/lib/assert/assert.h @@ -0,0 +1,98 @@ +/* + * Copyright (c) 2021 David Lamparter, for NetDEF, Inc. + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +/* WARNING: this file is "special" in that it overrides the system-provided + * assert.h by being on the include path before it. That means it should + * provide the functional equivalent. + * + * This is intentional because FRR extends assert() to write to the log and + * add backtraces. Overriding the entire file is the simplest and most + * reliable way to get this to work; there were problems previously with the + * system assert.h getting included afterwards and redefining assert() back to + * the system variant. + */ + +#ifndef _FRR_ASSERT_H +#define _FRR_ASSERT_H + +#include "xref.h" + +#ifdef __cplusplus +extern "C" { +#endif + +#ifndef __cplusplus +/* C++ has this built-in, but C provides it in assert.h for >=C11. Since we + * replace assert.h entirely, we need to provide it here too. + */ +#define static_assert _Static_assert +#endif + +struct xref_assert { + struct xref xref; + + const char *expr; + const char *extra, *args; +}; + +extern void _zlog_assert_failed(const struct xref_assert *xref, + const char *extra, ...) PRINTFRR(2, 3) + __attribute__((noreturn)); + +/* the "do { } while (expr_)" is there to get a warning for assignments inside + * the assert expression aka "assert(x = 1)". The (necessary) braces around + * expr_ in the if () statement would suppress these warnings. Since + * _zlog_assert_failed() is noreturn, the while condition will never be + * checked. + */ +#define assert(expr_) \ + ({ \ + static const struct xref_assert _xref __attribute__( \ + (used)) = { \ + .xref = XREF_INIT(XREFT_ASSERT, NULL, __func__), \ + .expr = #expr_, \ + }; \ + XREF_LINK(_xref.xref); \ + if (__builtin_expect((expr_) ? 0 : 1, 0)) \ + do { \ + _zlog_assert_failed(&_xref, NULL); \ + } while (expr_); \ + }) + +#define assertf(expr_, extra_, ...) \ + ({ \ + static const struct xref_assert _xref __attribute__( \ + (used)) = { \ + .xref = XREF_INIT(XREFT_ASSERT, NULL, __func__), \ + .expr = #expr_, \ + .extra = extra_, \ + .args = #__VA_ARGS__, \ + }; \ + XREF_LINK(_xref.xref); \ + if (__builtin_expect((expr_) ? 0 : 1, 0)) \ + do { \ + _zlog_assert_failed(&_xref, extra_, \ + ##__VA_ARGS__); \ + } while (expr_); \ + }) + +#define zassert assert + +#ifdef __cplusplus +} +#endif + +#endif /* _FRR_ASSERT_H */ diff --git a/lib/clippy.c b/lib/clippy.c index f1923d2a56..7ca99c9a94 100644 --- a/lib/clippy.c +++ b/lib/clippy.c @@ -115,15 +115,6 @@ void vzlogx(const struct xref_logmsg *xref, int prio, fputs("\n", stderr); } -void _zlog_assert_failed(const char *assertion, const char *file, - unsigned int line, const char *function) -{ - fprintf(stderr, - "Assertion `%s' failed in file %s, line %u, function %s", - assertion, file, line, (function ? function : "?")); - abort(); -} - void memory_oom(size_t size, const char *name) { abort(); diff --git a/lib/log.c b/lib/log.c index ca2f501686..15fdfd4b0a 100644 --- a/lib/log.c +++ b/lib/log.c @@ -311,17 +311,6 @@ void zlog_thread_info(int log_level) zlog(log_level, "Current thread not known/applicable"); } -void _zlog_assert_failed(const char *assertion, const char *file, - unsigned int line, const char *function) -{ - zlog(LOG_CRIT, "Assertion `%s' failed in file %s, line %u, function %s", - assertion, file, line, (function ? function : "?")); - zlog_backtrace(LOG_CRIT); - zlog_thread_info(LOG_CRIT); - log_memstats(stderr, "log"); - abort(); -} - void memory_oom(size_t size, const char *name) { zlog(LOG_CRIT, diff --git a/lib/subdir.am b/lib/subdir.am index fcaae9628a..480c2938d0 100644 --- a/lib/subdir.am +++ b/lib/subdir.am @@ -284,6 +284,8 @@ pkginclude_HEADERS += \ lib/zlog_targets.h \ lib/pbr.h \ lib/routing_nb.h \ + \ + lib/assert/assert.h \ # end @@ -412,7 +414,7 @@ lib_grammar_sandbox_SOURCES = \ lib_grammar_sandbox_LDADD = \ lib/libfrr.la -lib_clippy_CPPFLAGS = $(AM_CPPFLAGS) -D_GNU_SOURCE -DBUILDING_CLIPPY +lib_clippy_CPPFLAGS = $(CPPFLAGS_BASE) -D_GNU_SOURCE -DBUILDING_CLIPPY lib_clippy_CFLAGS = $(AC_CFLAGS) $(PYTHON_CFLAGS) lib_clippy_LDADD = $(PYTHON_LIBS) $(UST_LIBS) -lelf lib_clippy_LDFLAGS = -export-dynamic diff --git a/lib/xref.h b/lib/xref.h index 63166b069a..949458b313 100644 --- a/lib/xref.h +++ b/lib/xref.h @@ -33,6 +33,7 @@ enum xref_type { XREFT_THREADSCHED = 0x100, XREFT_LOGMSG = 0x200, + XREFT_ASSERT = 0x280, XREFT_DEFUN = 0x300, XREFT_INSTALL_ELEMENT = 0x301, diff --git a/lib/zlog.c b/lib/zlog.c index 24800c6e64..d2851c6028 100644 --- a/lib/zlog.c +++ b/lib/zlog.c @@ -521,6 +521,36 @@ void zlog_sigsafe(const char *text, size_t len) } } +void _zlog_assert_failed(const struct xref_assert *xref, const char *extra, ...) +{ + va_list ap; + static bool assert_in_assert; /* "global-ish" variable, init to 0 */ + + if (assert_in_assert) + abort(); + assert_in_assert = true; + + if (extra) { + struct va_format vaf; + + va_start(ap, extra); + vaf.fmt = extra; + vaf.va = ≈ + + zlog(LOG_CRIT, + "%s:%d: %s(): assertion (%s) failed, extra info: %pVA", + xref->xref.file, xref->xref.line, xref->xref.func, + xref->expr, &vaf); + + va_end(ap); + } else + zlog(LOG_CRIT, "%s:%d: %s(): assertion (%s) failed", + xref->xref.file, xref->xref.line, xref->xref.func, + xref->expr); + + /* abort() prints backtrace & memstats in SIGABRT handler */ + abort(); +} int zlog_msg_prio(struct zlog_msg *msg) { diff --git a/tests/.gitignore b/tests/.gitignore index ca20b0ecac..0c938beab6 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -21,6 +21,7 @@ /lib/cli/test_commands_defun.c /lib/northbound/test_oper_data /lib/cxxcompat +/lib/test_assert /lib/test_atomlist /lib/test_buffer /lib/test_checksum @@ -52,4 +53,4 @@ /lib/test_zmq /ospf6d/test_lsdb /ospf6d/test_lsdb_clippy.c -/zebra/test_lm_plugin \ No newline at end of file +/zebra/test_lm_plugin diff --git a/tests/lib/test_assert.c b/tests/lib/test_assert.c new file mode 100644 index 0000000000..8f1f4f2bad --- /dev/null +++ b/tests/lib/test_assert.c @@ -0,0 +1,64 @@ +/* + * Quick test for assert() + * Copyright (C) 2021 David Lamparter for NetDEF, Inc. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; see the file COPYING; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +/* make sure this works with assert.h & nothing else. also check the include + * shadowing, we don't want to pick up system assert.h + */ +#include + +__attribute__((noinline)) +void func_for_bt(int number) +{ + assert(number > 2); + assertf(number > 3, "(A) the number was %d", number); +} + +#include +#include "lib/zlog.h" +#include "lib/thread.h" +#include "lib/sigevent.h" + +int main(int argc, char **argv) +{ + int number = 10; + struct thread_master *master; + + zlog_aux_init("NONE: ", LOG_DEBUG); + + if (argc > 1) + number = atoi(argv[1]); + + assert(number > 0); + assertf(number > 1, "(B) the number was %d", number); + + /* set up SIGABRT handler */ + master = thread_master_create("test"); + signal_init(master, 0, NULL); + + func_for_bt(number); + assert(number > 4); + assertf(number > 5, "(C) the number was %d", number); + + assertf(number > 10, "(D) the number was %d", number); + return 0; +} diff --git a/tests/lib/test_assert.py b/tests/lib/test_assert.py new file mode 100644 index 0000000000..67c88e6220 --- /dev/null +++ b/tests/lib/test_assert.py @@ -0,0 +1,56 @@ +import frrtest +import os +import re +import subprocess +import inspect + +basedir = os.path.dirname(__file__) +program = os.path.join(basedir, "test_assert") + + +def check(number, rex=None): + proc = subprocess.Popen( + [frrtest.binpath(program), str(number)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + out, err = proc.communicate() + exitcode = proc.wait() + + if rex is None: + assert exitcode == 0 + else: + assert exitcode != 0 + + text = out.decode("US-ASCII") + err.decode("US-ASCII") + rex = re.compile(rex, re.M | re.S) + m = rex.search(text) + assert m is not None, "non-matching output: %s" % text + + +def test_assert_0(): + check(0, r"test_assert\.c:\d+.*number > 0") + + +def test_assert_1(): + check(1, r"test_assert\.c:\d+.*number > 1.*\(B\) the number was 1") + + +def test_assert_2(): + check(2, r"test_assert\.c:\d+.*number > 2") + + +def test_assert_3(): + check(3, r"test_assert\.c:\d+.*number > 3.*\(A\) the number was 3") + + +def test_assert_4(): + check(4, r"test_assert\.c:\d+.*number > 4") + + +def test_assert_10(): + check(10, r"test_assert\.c:\d+.*number > 10.*\(D\) the number was 10") + + +def test_assert_11(): + check(11) diff --git a/tests/subdir.am b/tests/subdir.am index ec0a154a2d..7384d9d96e 100644 --- a/tests/subdir.am +++ b/tests/subdir.am @@ -66,6 +66,7 @@ clippy_scan += \ check_PROGRAMS = \ tests/lib/cxxcompat \ + tests/lib/test_assert \ tests/lib/test_atomlist \ tests/lib/test_buffer \ tests/lib/test_checksum \ @@ -249,6 +250,10 @@ tests_lib_northbound_test_oper_data_CPPFLAGS = $(TESTS_CPPFLAGS) tests_lib_northbound_test_oper_data_LDADD = $(ALL_TESTS_LDADD) tests_lib_northbound_test_oper_data_SOURCES = tests/lib/northbound/test_oper_data.c nodist_tests_lib_northbound_test_oper_data_SOURCES = yang/frr-test-module.yang.c +tests_lib_test_assert_CFLAGS = $(TESTS_CFLAGS) +tests_lib_test_assert_CPPFLAGS = $(TESTS_CPPFLAGS) +tests_lib_test_assert_LDADD = $(ALL_TESTS_LDADD) +tests_lib_test_assert_SOURCES = tests/lib/test_assert.c tests_lib_test_atomlist_CFLAGS = $(TESTS_CFLAGS) tests_lib_test_atomlist_CPPFLAGS = $(TESTS_CPPFLAGS) tests_lib_test_atomlist_LDADD = $(ALL_TESTS_LDADD) @@ -404,6 +409,7 @@ EXTRA_DIST += \ tests/lib/northbound/test_oper_data.in \ tests/lib/northbound/test_oper_data.py \ tests/lib/northbound/test_oper_data.refout \ + tests/lib/test_assert.py \ tests/lib/test_atomlist.py \ tests/lib/test_nexthop_iter.py \ tests/lib/test_ntop.py \ diff --git a/tools/subdir.am b/tools/subdir.am index e159d82d4c..6a03a23baa 100644 --- a/tools/subdir.am +++ b/tools/subdir.am @@ -34,9 +34,11 @@ tools_gen_yang_deviations_SOURCES = tools/gen_yang_deviations.c tools_gen_yang_deviations_LDADD = lib/libfrr.la $(LIBYANG_LIBS) tools_ssd_SOURCES = tools/start-stop-daemon.c +tools_ssd_CPPFLAGS = # don't bother autoconf'ing these for a simple optional tool llvm_version = $(shell echo __clang_major__ | $(CC) -xc -P -E -) +tools_frr_llvm_cg_CPPFLAGS = $(CPPFLAGS_BASE) tools_frr_llvm_cg_CFLAGS = $(AM_CFLAGS) `llvm-config-$(llvm_version) --cflags` tools_frr_llvm_cg_LDFLAGS = `llvm-config-$(llvm_version) --ldflags --libs` tools_frr_llvm_cg_SOURCES = \