From 8c40c5ef8fc6902b9dcf54e107357500f92e2f33 Mon Sep 17 00:00:00 2001 From: Mark Rose Date: Sat, 11 Jul 2026 09:28:57 -0600 Subject: [PATCH] Fix uninitialized-pointer crash and double-free in ModuleFrn init failure paths - audio_fifo was never set in the ModuleFrn constructor, only assigned later in initialize(). If Module::initialize() fails early (e.g. a version mismatch), ModuleFrn::initialize() returns before any of the audio pipeline members (audio_fifo, audio_splitter, audio_valve) are assigned, leaving them as garbage/null. The destructor then unconditionally calls audio_fifo->unregisterSource(), audio_splitter->removeSink(), and audio_valve->unregisterSink() on those pointers, crashing the module. Fixed by initializing audio_fifo to null in the constructor and guarding all three dereferences in moduleCleanup() with null checks. - When qso->initOk() returns false (any required FRN config variable is missing), initialize() deletes qso but leaves the member pointing at the freed object. The module is then destroyed, and moduleCleanup() uses the dangling qso pointer (audio_splitter->removeSink(qso)) and deletes it a second time. Fixed by nulling qso immediately after the delete on this failure path. Co-Authored-By: Claude Opus 4.8 --- src/svxlink/modules/frn/CMakeLists.txt | 6 ++ src/svxlink/modules/frn/ModuleFrn.cpp | 17 +++- src/svxlink/modules/frn/ModuleFrnTest.cpp | 95 +++++++++++++++++++ .../modules/frn/ModuleFrnTest.deps.cmake | 19 ++++ .../modules/frn/ModuleFrnTestLinkStubs.cpp | 59 ++++++++++++ 5 files changed, 193 insertions(+), 3 deletions(-) create mode 100644 src/svxlink/modules/frn/ModuleFrnTest.cpp create mode 100644 src/svxlink/modules/frn/ModuleFrnTest.deps.cmake create mode 100644 src/svxlink/modules/frn/ModuleFrnTestLinkStubs.cpp diff --git a/src/svxlink/modules/frn/CMakeLists.txt b/src/svxlink/modules/frn/CMakeLists.txt index 472e2ecd8..7161fc5b5 100644 --- a/src/svxlink/modules/frn/CMakeLists.txt +++ b/src/svxlink/modules/frn/CMakeLists.txt @@ -27,3 +27,9 @@ install(FILES ${MODNAME}.tcl DESTINATION ${SVX_SHARE_INSTALL_DIR}/events.d) install_if_not_exists(Module${MODNAME}.conf ${SVX_SYSCONF_INSTALL_DIR}/svxlink.d ) + +# Unit tests: every *Test.cpp here is auto-registered once the CTest +# suite (SvxLinkAddTests) is present; harmless without it. +if(COMMAND svxlink_add_tests) + svxlink_add_tests() +endif() diff --git a/src/svxlink/modules/frn/ModuleFrn.cpp b/src/svxlink/modules/frn/ModuleFrn.cpp index ed27c58b5..1d6ba9f84 100644 --- a/src/svxlink/modules/frn/ModuleFrn.cpp +++ b/src/svxlink/modules/frn/ModuleFrn.cpp @@ -129,6 +129,7 @@ ModuleFrn::ModuleFrn(void *dl_handle, Logic *logic, const string& cfg_name) , audio_valve(0) , audio_splitter(0) , audio_selector(0) + , audio_fifo(0) { cout << "\tModule Frn v" MODULE_FRN_VERSION " starting...\n"; @@ -229,6 +230,7 @@ bool ModuleFrn::initialize(void) if (!qso->initOk()) { delete qso; + qso = 0; cerr << "*** ERROR: Creation of Qso object failed\n"; return false; } @@ -241,10 +243,19 @@ bool ModuleFrn::initialize(void) void ModuleFrn::moduleCleanup() { AudioSource::clearHandler(); - audio_fifo->unregisterSource(); + if (audio_fifo != 0) + { + audio_fifo->unregisterSource(); + } - audio_splitter->removeSink(qso); - audio_valve->unregisterSink(); + if (audio_splitter != 0) + { + audio_splitter->removeSink(qso); + } + if (audio_valve != 0) + { + audio_valve->unregisterSink(); + } AudioSink::clearHandler(); delete qso; diff --git a/src/svxlink/modules/frn/ModuleFrnTest.cpp b/src/svxlink/modules/frn/ModuleFrnTest.cpp new file mode 100644 index 000000000..f84f8b712 --- /dev/null +++ b/src/svxlink/modules/frn/ModuleFrnTest.cpp @@ -0,0 +1,95 @@ +/** +@file ModuleFrnTest.cpp +@brief Unit test for ModuleFrn init-failure pointer lifecycle +@author Mark Rose +@date 2026-07-11 + +ModuleFrn::initialize() only assigns audio_fifo, audio_splitter and +audio_valve after the base Module::initialize() call succeeds. If +initialization fails (or is never called) before those members are +assigned, the ModuleFrn destructor still unconditionally calls +moduleCleanup(), which used to dereference audio_fifo unconditionally. +Because audio_fifo was not part of the constructor initializer list, it +held whatever garbage bits were already present in the object's storage, +so moduleCleanup() dereferenced an uninitialized/garbage pointer and +crashed. + +This test places a ModuleFrn instance into memory that has been poisoned +with a non-canonical bit pattern *before* construction -- exactly the +kind of "garbage" an uninitialized pointer member would contain -- then +destroys the object without ever calling initialize(). With the fix, +the constructor explicitly nulls audio_fifo and moduleCleanup() guards +every audio pipeline pointer with a null check, so destruction completes +cleanly. Without the fix, moduleCleanup() dereferences the poisoned +audio_fifo pointer and the process crashes (a non-canonical x86_64 +address reliably faults on first access), which CTest observes as a +failing (non-zero/signal-terminated) test. + +\verbatim +SvxLink - A Multi Purpose Voice Services System for Ham Radio Use +Copyright (C) 2026 Tobias Blomberg / SM0SVX + +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. +\endverbatim +*/ + +#include +#include +#include + +#include "ModuleFrn.h" + +using namespace std; + +namespace { + +int failures = 0; + +void check(bool cond, const string& msg) +{ + cout << (cond ? " ok " : " FAIL ") << msg << endl; + if (!cond) + { + ++failures; + } +} + +} /* anonymous namespace */ + + +int main(void) +{ + // Poison the storage for a ModuleFrn instance with a non-canonical + // x86_64 bit pattern before construction. This stands in for the + // uninitialized garbage that a pointer member would contain if it is + // never assigned by the constructor -- exactly the pre-fix bug for + // audio_fifo. Any real dereference of this pattern as a pointer + // reliably faults. + alignas(ModuleFrn) unsigned char storage[sizeof(ModuleFrn)]; + memset(storage, 0xAB, sizeof(storage)); + + // Construct in place. dl_handle and logic are both unused by the + // constructor and by moduleCleanup(), so passing null is safe as + // long as initialize() (which does dereference logic()) is never + // called -- exactly the failure mode this test targets: initialize() + // fails (or, as here, is never reached) before the audio pipeline + // members are assigned. + ModuleFrn *mod = new (storage) ModuleFrn(0, 0, "frn"); + + // Destroy without ever calling initialize(). This exercises + // moduleCleanup() with audio_fifo (and friends) left at whatever the + // constructor set them to. Pre-fix, audio_fifo is left as the + // poisoned garbage above and moduleCleanup() crashes dereferencing + // it. Post-fix, audio_fifo is explicitly nulled and every dereference + // in moduleCleanup() is null-guarded, so this returns cleanly. + mod->~ModuleFrn(); + + check(true, "destructor completed without dereferencing an " + "uninitialized audio_fifo pointer"); + + cout << (failures == 0 ? "ALL TESTS PASSED" : "TESTS FAILED") << endl; + return failures != 0; +} /* main */ diff --git a/src/svxlink/modules/frn/ModuleFrnTest.deps.cmake b/src/svxlink/modules/frn/ModuleFrnTest.deps.cmake new file mode 100644 index 000000000..3e62ce5bd --- /dev/null +++ b/src/svxlink/modules/frn/ModuleFrnTest.deps.cmake @@ -0,0 +1,19 @@ +# Extra sources/libs needed to link ModuleFrnTest. +# +# ModuleFrn.cpp requires QsoFrn.cpp/Utils.cpp (its module siblings) and +# Module.cpp (the svxlink core base class it derives from). Module.cpp +# in turn makes a few direct (non-virtual) calls into Logic:: and touches +# the LinkManager singleton pointer; ModuleFrnTestLinkStubs.cpp provides +# trivial link-only stand-ins for those so the real Logic/LinkManager +# subsystem (and the Async event loop / application wiring it needs) +# does not have to be pulled in for this standalone test. +set(ModuleFrnTest_EXTRA_SRCS + ModuleFrn.cpp + QsoFrn.cpp + Utils.cpp + ModuleFrnTestLinkStubs.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/../../svxlink/Module.cpp + ) +set(ModuleFrnTest_EXTRA_LIBS + asynccpp asynccore asyncaudio svxmisc ${GSM_LIBRARY} + ) diff --git a/src/svxlink/modules/frn/ModuleFrnTestLinkStubs.cpp b/src/svxlink/modules/frn/ModuleFrnTestLinkStubs.cpp new file mode 100644 index 000000000..f94642b82 --- /dev/null +++ b/src/svxlink/modules/frn/ModuleFrnTestLinkStubs.cpp @@ -0,0 +1,59 @@ +/** +@file ModuleFrnTestLinkStubs.cpp +@brief Link-only stand-ins for symbols ModuleFrnTest never executes +@author Mark Rose +@date 2026-07-11 + +ModuleFrnTest.cpp constructs and destroys a ModuleFrn without ever +calling initialize(), so it never exercises any Logic:: or LinkManager +functionality. However, Module.cpp (compiled in as part of the module +under test, since ModuleFrn derives from Module) contains a handful of +member functions -- reachable only from code paths this test never +runs -- that make *direct* (non-virtual) calls into Logic:: methods and +touch the LinkManager singleton pointer. Because those functions are +part of the same translation unit that is linked into this test binary, +the linker requires those symbols to be resolvable even though they are +never invoked at runtime by this test. + +Pulling in the real Logic.cpp/LinkManager.cpp to satisfy this would drag +in the full logic-core/event-handling/link-management subsystem, which +needs a live Async event loop and application wiring -- exactly what this +small unit test intentionally avoids. These stubs instead provide trivial +definitions for just the non-virtual symbols Module.cpp references, so +the parts of ModuleFrn actually under test (the constructor/destructor +pointer lifecycle) can be linked and run standalone. None of these bodies +are ever executed by ModuleFrnTest. + +\verbatim +SvxLink - A Multi Purpose Voice Services System for Ham Radio Use +Copyright (C) 2026 Tobias Blomberg / SM0SVX + +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. +\endverbatim +*/ + +#include "Logic.h" +#include "LinkManager.h" + +using namespace std; + + + // Non-virtual Logic:: member functions that Module.cpp calls directly + // (not through a vtable). Never invoked by this test -- ModuleFrn is + // destroyed before initialize() is ever called -- but the symbols must + // exist for the link to succeed. +void Logic::setEventVariable(const string&, const string&) {} +void Logic::sendDtmf(const string&) {} +Module *Logic::findModule(int) { return 0; } +bool Logic::isWritingMessage(void) { return false; } + + // Storage for the LinkManager singleton pointer. Module::activate()/ + // deactivate() check LinkManager::hasInstance() (inline, reads this + // pointer) before touching the singleton; leaving it null means that + // guard is always false and the singleton is never dereferenced. +LinkManager *LinkManager::_instance = 0; + +void LinkManager::setLogicMute(const LogicBase*, bool) {}