]> git.puffer.fish Git - mirror/frr.git/commitdiff
lib: rework how we "override" assert()
authorDavid Lamparter <equinox@diac24.net>
Tue, 16 Mar 2021 10:03:44 +0000 (11:03 +0100)
committerDavid Lamparter <equinox@opensourcerouting.org>
Sun, 2 May 2021 14:27:17 +0000 (16:27 +0200)
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 <equinox@opensourcerouting.org>
12 files changed:
Makefile.am
lib/assert/assert.h [new file with mode: 0644]
lib/clippy.c
lib/log.c
lib/subdir.am
lib/xref.h
lib/zlog.c
tests/.gitignore
tests/lib/test_assert.c [new file with mode: 0644]
tests/lib/test_assert.py [new file with mode: 0644]
tests/subdir.am
tools/subdir.am

index e4149b62ed4129c3e964517aad30eaec9d354587..a5101df2f0eec2b11f4e199e4d953af42b692f1b 100644 (file)
@@ -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 (file)
index 0000000..fbdbd52
--- /dev/null
@@ -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 */
index f1923d2a5629ab3d6a474bcae701f8d2bc97e171..7ca99c9a940191c45597c12dc3081abe71d96ac1 100644 (file)
@@ -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();
index ca2f50168602638a34cb600eb32be1f4db72bb41..15fdfd4b0a480da9a6d323db1b0ed6284297653d 100644 (file)
--- 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,
index fcaae9628a581a013cbeb323c39bd26d988d6284..480c2938d0d4a93b3668f2bd0a01e8dc43daec4f 100644 (file)
@@ -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
index 63166b069a3811502363a1dd869197003fa9dec3..949458b313a334ef26d9e791a63aaec244a47871 100644 (file)
@@ -33,6 +33,7 @@ enum xref_type {
        XREFT_THREADSCHED = 0x100,
 
        XREFT_LOGMSG = 0x200,
+       XREFT_ASSERT = 0x280,
 
        XREFT_DEFUN = 0x300,
        XREFT_INSTALL_ELEMENT = 0x301,
index 24800c6e64edad30087e3d4e709e32e39db230c4..d2851c6028b57878b70f70657066781144c8dced 100644 (file)
@@ -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 = &ap;
+
+               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)
 {
index ca20b0ecac2c5f48ae3df51c98f304bbb356fa3b..0c938beab67a5bfbce28b6a580e4d6ab4fd325ee 100644 (file)
@@ -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 (file)
index 0000000..8f1f4f2
--- /dev/null
@@ -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 <assert.h>
+
+__attribute__((noinline))
+void func_for_bt(int number)
+{
+       assert(number > 2);
+       assertf(number > 3, "(A) the number was %d", number);
+}
+
+#include <zebra.h>
+#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 (file)
index 0000000..67c88e6
--- /dev/null
@@ -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)
index ec0a154a2d83bff88f6abbf56ce960b1fe564ad8..7384d9d96e27adf1ca0cc361a332756334b0d6e1 100644 (file)
@@ -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 \
index e159d82d4cd2d22d01dc403ea651a17f620d4a7e..6a03a23baa7b33f0ebd47e4823a8b9edba37317f 100644 (file)
@@ -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 = \