Fix destruct ub and ubsan ci

.github/workflows/ci.yml

@@ -9,13 +9,25 @@ on:
 jobs:
   build-and-test:
     runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        compiler: [gcc, clang]
     steps:
       - uses: actions/checkout@v6
 
       - name: Install dependencies
-        run: sudo apt-get update && sudo apt-get install -y libpcap-dev
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y libpcap-dev
+          if [ "${{ matrix.compiler }}" = "clang" ]; then
+            sudo apt-get install -y clang
+          fi
 
       - name: Configure
+        env:
+          CC: ${{ matrix.compiler }}
+          CXX: ${{ matrix.compiler == 'clang' && 'clang++' || 'g++' }}
         run: cmake -DTEST_ENABLE=on -B build
 
       - name: Build

CMakeLists.txt

@@ -4,6 +4,12 @@ cmake_minimum_required(VERSION 3.20)
 option(TEST_ENABLE "Enable tests" off)
 option(FUZZ_ENABLE "Enable fuzz targets (requires clang with libFuzzer)" off)
 
+# libpcap is optional. Disabling it drops pcap-format input/output (the
+# `pcap` command and pcap capture) but keeps everything else. Useful when
+# linking under sanitizers that flag uninitialized reads in transitive
+# dependencies of libpcap (e.g. libnl-route-3 DSO init under MSan).
+option(EF_USE_LIBPCAP "Detect and link libpcap if available" ON)
+
 if (${TEST_ENABLE})
     project(easyframes)
 else()
@@ -15,20 +21,22 @@ include_directories(src)
 
 
 set(_LIBPCAP "")
-find_package(PkgConfig)
-pkg_check_modules(PCAP libpcap)
-if (PCAP_FOUND)
-    add_definitions(-DHAS_LIBPCAP)
-    include_directories(${PCAP_INCLUDE_DIRS})
-    set(_LIBPCAP ${PCAP_LIBRARIES})
-else()
-    FIND_PATH(PCAP_INCLUDE_DIR NAMES pcap/pcap.h)
-    FIND_LIBRARY(PCAP_LIBRARY NAMES pcap)
-
-    if (PCAP_LIBRARY)
+if (EF_USE_LIBPCAP)
+    find_package(PkgConfig)
+    pkg_check_modules(PCAP libpcap)
+    if (PCAP_FOUND)
         add_definitions(-DHAS_LIBPCAP)
-        include_directories(${PCAP_INCLUDE_DIR})
-        set(_LIBPCAP ${PCAP_LIBRARY})
+        include_directories(${PCAP_INCLUDE_DIRS})
+        set(_LIBPCAP ${PCAP_LIBRARIES})
+    else()
+        FIND_PATH(PCAP_INCLUDE_DIR NAMES pcap/pcap.h)
+        FIND_LIBRARY(PCAP_LIBRARY NAMES pcap)
+
+        if (PCAP_LIBRARY)
+            add_definitions(-DHAS_LIBPCAP)
+            include_directories(${PCAP_INCLUDE_DIR})
+            set(_LIBPCAP ${PCAP_LIBRARY})
+        endif()
     endif()
 endif()
 
@@ -92,13 +100,31 @@ set(CMAKE_CXX_STANDARD 14)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
 set(CMAKE_CXX_EXTENSIONS OFF)
 
+# Run the test build under UBSan with recovery disabled so any undefined
+# behaviour fails CI loudly. -fsanitize=function in particular catches
+# indirect calls through function-pointer types incompatible with the
+# pointee's actual signature (C99/C11 §6.3.2.3); see commit history for
+# the destruct_free regression this guards against. The function check
+# is clang-only, so probe for it.
+include(CheckCCompilerFlag)
+set(EF_SANITIZE_FLAGS -fsanitize=undefined -fno-sanitize-recover=all -fno-omit-frame-pointer)
+check_c_compiler_flag(-fsanitize=function EF_HAVE_FSANITIZE_FUNCTION)
+if (EF_HAVE_FSANITIZE_FUNCTION)
+    list(APPEND EF_SANITIZE_FLAGS -fsanitize=function)
+endif()
+target_compile_options(libef PRIVATE ${EF_SANITIZE_FLAGS})
+target_link_options(libef PUBLIC ${EF_SANITIZE_FLAGS})
+
 add_executable(ef-tests
     test/ef-test.cxx
     test/ef-tests.cxx
     test/test-ef-parse-bytes.cxx
     test/ifh-ignore.cxx
     test/test-padding.cxx
+    test/test-alloc-free.cxx
+    test/test-mld-zero-width.cxx
 )
+target_compile_options(ef-tests PRIVATE ${EF_SANITIZE_FLAGS})
 
 target_link_libraries(ef-tests libef)
 include(CTest)

src/ef.c

@@ -28,18 +28,6 @@ void print_hex_str(int fd, void *_d, int s) {
     }
 }
 
-typedef void (*destruct_cb_t)(void *buf);
-
-void destruct_free(void *buf, void *cb_) {
-    destruct_cb_t cb = (destruct_cb_t)cb_;
-    if (!buf)
-        return;
-
-    cb(buf);
-    free(buf);
-}
-
-
 void field_destruct(field_t *f) {
     if (!f)
         return;
@@ -292,6 +280,8 @@ buf_t *frame_def(hdr_t *hdr) {
         field_t *f = &hdr->fields[i];
         if (!f->def)
             continue;
+        if (f->bit_width == 0)
+            continue;
 
         hdr_write_field(b, 0, f, f->def);
     }

src/ef.h

@@ -77,11 +77,12 @@ size_t bwrite_all(int fd, const buf_list_t *buf);
 
 ///////////////////////////////////////////////////////////////////////////////
 
-void destruct_free(void *buf, void *cb);
-
 #define GEN_ALLOC_CLONE_FREE(name)                                             \
 static inline void name ## _free(name ## _t *f) {                              \
-    destruct_free(f, (void *)&name ## _destruct);                              \
+    if (!f)                                                                    \
+        return;                                                                \
+    name ## _destruct(f);                                                      \
+    free(f);                                                                   \
 }                                                                              \
 static inline name ## _t *name ## _alloc() {                                   \
     return (name ## _t *)calloc(1, sizeof(name ## _t));                        \

test/test-alloc-free.cxx

@@ -0,0 +1,39 @@
+#include "ef.h"
+#include "ef-test.h"
+#include "catch_single_include.hxx"
+
+// These tests deliberately drive every name##_free path generated by
+// GEN_ALLOC_CLONE_FREE. Under UBSan -fsanitize=function this catches any
+// future regression where the destructor signature stops matching the
+// indirect-call type used by name##_free (the bug fixed by inlining the
+// macro body to call name##_destruct directly).
+
+TEST_CASE("field_free dispatches to field_destruct", "[alloc-free]") {
+    field_t *f = field_alloc();
+    REQUIRE(f != nullptr);
+    field_free(f);
+    field_free(nullptr);  // null-tolerant
+}
+
+TEST_CASE("hdr_free dispatches to hdr_destruct", "[alloc-free]") {
+    hdr_t *h = hdr_alloc();
+    REQUIRE(h != nullptr);
+    hdr_free(h);
+    hdr_free(nullptr);
+}
+
+TEST_CASE("frame_free dispatches to frame_destruct", "[alloc-free]") {
+    frame_t *f = frame_alloc();
+    REQUIRE(f != nullptr);
+    frame_free(f);
+    frame_free(nullptr);
+}
+
+TEST_CASE("frame_free of a populated frame releases the stack", "[alloc-free]") {
+    // Exercises the recursive hdr_free -> field_free chain so UBSan also
+    // sees the indirect calls via the cloned templates' destructors.
+    frame_t *f = parse_frame_wrap({"eth", "dmac", "00:00:00:00:00:01",
+                                   "smac", "00:00:00:00:00:02"});
+    REQUIRE(f != nullptr);
+    frame_free(f);
+}

test/test-mld-zero-width.cxx

@@ -0,0 +1,61 @@
+#include "ef.h"
+#include "ef-test.h"
+#include "catch_single_include.hxx"
+
+// Regression for the v2-query shrink path in mld_fill_defaults: when
+// rresv/ng widths are zeroed but their bit_offsets (224, 240) are left
+// pointing past the shrunk 28-byte MLD header, frame_mask_to_buf used
+// to assert in hdr_write_field because the mask path force-allocates
+// a maskb and writes it for every field regardless of width.
+//
+// hdr_copy_to_buf_ now skips bit_width==0 fields, so building a mask
+// for an MLDv2 query no longer trips the assert. frame_to_buf was
+// always fine (val/def NULL paths skip the write), but we cover both.
+
+static const std::vector<const char *> mldv2_query = {
+    "eth", "dmac", "33:33:00:00:00:01",
+           "smac", "00:00:00:00:00:01",
+    "ipv6", "hlim", "1", "sip", "::", "dip", "ff02::1",
+    "data", "hex", "3a00050200000100",
+    "mld", "type", "130", "max_resp", "10000",
+           "qrv", "2", "qqic", "125", "ns", "0",
+};
+
+TEST_CASE("frame_mask_to_buf does not assert on MLDv2 query (zero-width rresv/ng)",
+          "[mld][zero-width]") {
+    frame_t *f = parse_frame_wrap(mldv2_query);
+    REQUIRE(f != nullptr);
+
+    // Mirror what ef-args.c:261-265 does for rx patterns: frame_to_buf
+    // runs first and mld_fill_defaults shrinks the MLD header (size 32
+    // -> 28) and zeroes rresv/ng bit_widths. frame_mask_to_buf then
+    // observes that mutated state — and used to assert in hdr_write_field
+    // because the mask path force-allocates a maskb and writes it for
+    // every field regardless of width, with bit_offsets still pointing
+    // past the shrunk header end.
+    buf_t *val = frame_to_buf(f);
+    REQUIRE(val != nullptr);
+
+    buf_t *mask = frame_mask_to_buf(f);
+    REQUIRE(mask != nullptr);
+    CHECK(mask->size >= 14 + 40 + 8 + 28);  // eth+ipv6+data+shrunk-mld
+
+    bfree(val);
+    bfree(mask);
+    frame_free(f);
+}
+
+TEST_CASE("frame_to_buf builds an MLDv2 query without padding the shrunk fields",
+          "[mld][zero-width]") {
+    frame_t *f = parse_frame_wrap(mldv2_query);
+    REQUIRE(f != nullptr);
+
+    buf_t *buf = frame_to_buf(f);
+    REQUIRE(buf != nullptr);
+    // Frame is padded to 60 bytes by default; raw size before padding
+    // is eth(14) + ipv6(40) + data(8) + mld(28) = 90, > 60, so no pad.
+    CHECK(buf->size == 90);
+
+    bfree(buf);
+    frame_free(f);
+}