From 713e318bc36254caae15c7271172c7d9708d7bfb Mon Sep 17 00:00:00 2001 From: Igor Gudich Date: Tue, 19 May 2026 11:35:53 +0200 Subject: [PATCH 1/4] fix GEN_ALLOC_CLONE_FREE: drop UB indirection through destruct_free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit name##_free called destruct_free(f, &name##_destruct), which cast the typed destructor (e.g. void (*)(field_t *)) to void (*)(void *) and called through the latter. Per C99/C11 §6.3.2.3¶8, calling a function through a pointer of an incompatible type is undefined behaviour, even when the calling convention happens to match. clang's -fsanitize=function flags this on every *_free. Inline the destructor call in the macro body so the call goes through the function's actual type. destruct_free and destruct_cb_t become dead; remove them. --- src/ef.c | 12 ------------ src/ef.h | 7 ++++--- 2 files changed, 4 insertions(+), 15 deletions(-) diff --git a/src/ef.c b/src/ef.c index 383d6dc..b7546aa 100644 --- a/src/ef.c +++ b/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; diff --git a/src/ef.h b/src/ef.h index 85ad02c..c48a8b1 100644 --- a/src/ef.h +++ b/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)); \ From c360a718cc01dc92e0c3bd728f843a563b89752e Mon Sep 17 00:00:00 2001 From: Igor Gudich Date: Tue, 19 May 2026 11:36:04 +0200 Subject: [PATCH 2/4] ci: run test build under UBSan and add a clang job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Build libef and ef-tests with -fsanitize=undefined -fno-sanitize-recover=all under TEST_ENABLE so any UB hit at runtime aborts the test and fails CI. Probe for -fsanitize=function (clang-only) and add it when available; this catches indirect-call type mismatches like the destruct_free regression fixed in the previous commit. Add a [gcc, clang] matrix to the GitHub Actions workflow so the function check actually runs in CI. Add test/test-alloc-free.cxx exercising every name##_free path explicitly (field, hdr, frame — including the recursive frame_free -> hdr_free -> field_free chain on a populated frame) so UBSan covers them. --- .github/workflows/ci.yml | 14 +++++++++++++- CMakeLists.txt | 17 +++++++++++++++++ test/test-alloc-free.cxx | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 test/test-alloc-free.cxx diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 11af89c..02fe7fa 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/CMakeLists.txt b/CMakeLists.txt index ecb5075..137d20b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -92,13 +92,30 @@ 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 ) +target_compile_options(ef-tests PRIVATE ${EF_SANITIZE_FLAGS}) target_link_libraries(ef-tests libef) include(CTest) diff --git a/test/test-alloc-free.cxx b/test/test-alloc-free.cxx new file mode 100644 index 0000000..1fd20bf --- /dev/null +++ b/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); +} From d26df2999479e5e4aea7cb682bbe9b7e66121ae6 Mon Sep 17 00:00:00 2001 From: Igor Gudich Date: Tue, 19 May 2026 12:19:59 +0200 Subject: [PATCH 3/4] build: add EF_USE_LIBPCAP option to gate libpcap detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Default ON preserves existing behaviour. -DEF_USE_LIBPCAP=OFF skips the detect-and-link block entirely so libef and ef do not pull in libpcap or its transitive deps. Useful for sanitizer builds where a transitive dependency (e.g. libnl-route-3 brought in by libpcap) trips MSan on uninitialized DSO-init reads outside our control — previously fixable only by carrying a downstream patch that commented out the detection block. --- CMakeLists.txt | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 137d20b..9ed03f6 100644 --- a/CMakeLists.txt +++ b/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() From fe12a52789c7e6d78c858e6bb2f4b4987b8df527 Mon Sep 17 00:00:00 2001 From: Igor Gudich Date: Tue, 19 May 2026 12:04:18 +0200 Subject: [PATCH 4/4] fix frame_def + test: skip zero-width fields, MLDv2 query regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit frame_def's serialization loop has the same shape as hdr_copy_to_buf_ which 4d9ce68 just guarded: if a frame_fill_defaults hook zeroes a field's bit_width while leaving its bit_offset past the new header end, hdr_write_field would assert. Today no header sets a def_val on a width-0 field, so this is purely defensive — but symmetric with the call-site fix and harmless. Also add a Catch2 regression test mirroring the ef-args.c sequence that triggered the original mld_fill_defaults bug: parse an MLDv2 query (with qrv/qqic/ns set, no rresv/ng), call frame_to_buf to drive mld_fill_defaults' shrink + bit_width-zeroing, then call frame_mask_to_buf on the same now-mutated frame. Pre-fix the second call aborted in hdr_write_field; post-fix the suite passes. --- CMakeLists.txt | 1 + src/ef.c | 2 ++ test/test-mld-zero-width.cxx | 61 ++++++++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+) create mode 100644 test/test-mld-zero-width.cxx diff --git a/CMakeLists.txt b/CMakeLists.txt index 9ed03f6..e0fe60f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -122,6 +122,7 @@ add_executable(ef-tests 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}) diff --git a/src/ef.c b/src/ef.c index b7546aa..2d7060f 100644 --- a/src/ef.c +++ b/src/ef.c @@ -280,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); } diff --git a/test/test-mld-zero-width.cxx b/test/test-mld-zero-width.cxx new file mode 100644 index 0000000..3ad5e2e --- /dev/null +++ b/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 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); +}