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..e0fe60f 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() @@ -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) diff --git a/src/ef.c b/src/ef.c index 383d6dc..2d7060f 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; @@ -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); } 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)); \ 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); +} 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); +}