Files
firmware/test/test_safefile/test_main.cpp
Jonathan BennettandClaude Opus 5 e691bd3790 Claude/dmshell lock (#12024)
* Lock: give Portduino a real mutex instead of the empty fallback

Lock.cpp has a FreeRTOS implementation and an empty one, and Portduino takes
the empty one: every lock() and unlock() on a Linux build is a no-op, so
concurrency::Lock protects nothing there. TrafficManagementModule's cacheLock
and SPILock are both built on it, and native meshtasticd runs the radio and the
API on separate threads.

Add a pthread implementation under ARCH_PORTDUINO. The timed lock(uint32_t)
blocks rather than returning early, because there is no portable timed
pthread_mutex_lock across Linux and macOS and returning true without acquiring
would leave a caller such as SPILock unlocking a mutex it never took. Targets
that have neither FreeRTOS nor pthreads, such as STM32WL, keep the existing
empty implementation byte for byte.

Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9

* test: create spiLock in the shared setup, which NodeDB needs and no test had

Making Portduino's Lock real turns a latent null dereference into a crash. spiLock
is a bare pointer that initSPI() fills in, and only main.cpp calls that, so in a
test binary it stays null. NodeDB's constructor reaches it through loadFromDisk(),
and while Lock::lock() was an empty function the call never touched `this`, so
23 suites have been calling a method on a null pointer and getting away with it.
With a pthread mutex behind it the same call reads through the null pointer and
takes SIGSEGV at offset 0x10, which is what test_phone_api_config_dump,
test_muted_source, test_nodeinfo_send_window and test_module_config hit.

Create it once in initializeTestEnvironment(), which every affected suite calls as
the first statement of setup(), before any of them constructs a NodeDB. The guard
is the idiom test_xmodem and test_nodedb_identity_hygiene already use; theirs stay
correct and become no-ops. test_safefile called initSPI() bare right after the
harness, which would now trip its assert, so that call goes away.

No firmware behaviour changes: main.cpp still calls initSPI() exactly once, and
nothing outside the test harness is touched.

Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9

* test: create cryptLock where a suite reaches it without building a Router

Second instance of the same latent null dereference the previous commit fixed
for spiLock. cryptLock is a bare pointer that Router's constructor creates
(Router.cpp:246); AdminModule::setPassKey takes a LockGuard on it, and a suite
that exercises an admin path without standing up a Router leaves it null. While
Lock::lock() was empty on Portduino the guard never touched `this`; with a
pthread mutex it reads through null, which is test_tak_config's SIGSEGV in
handleGetModuleConfig.

It cannot go in initializeTestEnvironment() the way spiLock did, because Router
asserts cryptLock is unset before allocating its own, so creating it for every
suite would break the ones that do build a Router. It is a named helper instead,
testEnsureCryptLock(), called by the six suites that reach a cryptLock path with
no Router: test_ack_proof, test_admin_session_repro, test_fuzz_packets,
test_hop_scaling, test_module_config and test_tak_config. The three that define
setup() twice behind a PKI #if get the call only in the branch that compiles the
tests in.

No firmware behaviour changes; nothing outside test/ is touched.

Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9

* test: create cryptLock in the harness instead of chasing suites

Router's constructor asserted cryptLock was unset, then allocated it. That
assert is why ten suites carry a mock-router destructor whose only job is to
delete the global and null it so the next router can be built. While
Lock::lock() was an empty function on Portduino a null cryptLock cost nothing,
so those null windows were invisible; with a real mutex, anything reaching
perhapsDecode() or the ack-proof paths after one of those destructors runs
dereferences null.

The fix is the idiom already on the next line of the same constructor, which
routingAuthCacheLock has used all along: reuse the lock if one exists. Nothing
in src/ ever deleted cryptLock, so a Router that finds one is finding the
process's only one. initializeTestEnvironment() can then create it for every
suite, the way it now does for spiLock, and the ten teardowns and the
per-suite helper from the previous commit all go away.

Replaces the six testEnsureCryptLock() call sites with one creation point, and
removes the null windows in test_admin_radio, test_mesh_beacon,
test_mesh_module, test_mqtt, test_nexthop_routing, test_nodeinfo_send_window,
test_traffic_management, test_event_channel_phone_api,
test_event_channel_router and test_phone_api_config_dump.

Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9

---------

Co-authored-by: Claude <noreply@anthropic.com>
2026-10-01 21:33:14 +00:00

134 lines
3.7 KiB
C++

// Covers SafeFile's write-tmp / readback / rename-over path, on both the fullAtomic and the
// !fullAtomic construction. See the commit message for why these cannot go red on this host.
#include "MeshTypes.h"
#include "TestUtil.h"
// FSCommon.h is what defines FSCom, so it has to be included before anything tests for it.
#include "FSCommon.h"
#include "SPILock.h"
#include "SafeFile.h"
#include <cstring>
#include <string>
#include <unity.h>
#ifdef FSCom
// Built through FSCom rather than host fopen(): PortduinoFS confines paths to its own mountpoint, so
// the same API is the only way the test stays agnostic about where that mountpoint is.
static const char *kRoot = "/test_safefile";
static const char *kFile = "/test_safefile/pref.dat";
static const char *kTmp = "/test_safefile/pref.dat.tmp";
static void writeRaw(const char *path, const char *payload)
{
File f = FSCom.open(path, FILE_O_WRITE);
TEST_ASSERT_TRUE_MESSAGE(f, path);
f.write((const uint8_t *)payload, strlen(payload));
f.close();
}
static std::string readAll(const char *path)
{
std::string out;
File f = FSCom.open(path, FILE_O_READ);
if (!f)
return out;
int c;
while ((c = f.read()) >= 0)
out.push_back((char)c);
f.close();
return out;
}
static void saveThrough(bool fullAtomic, const char *payload)
{
auto f = SafeFile(kFile, fullAtomic);
f.write((const uint8_t *)payload, strlen(payload));
TEST_ASSERT_TRUE(f.close());
}
void setUp(void)
{
rmDir(kRoot);
FSCom.mkdir(kRoot);
}
void tearDown(void)
{
// Leave nothing behind: the sandbox is per suite, but an undeclared write is still a finding.
rmDir(kRoot);
}
// A .tmp surviving a reset between close() and renameFile() must not contribute a byte to the
// next save.
void test_stale_tmp_is_not_appended_to(void)
{
writeRaw(kTmp, "STALESTALESTALE");
saveThrough(true, "NEW");
TEST_ASSERT_EQUAL_STRING("NEW", readAll(kFile).c_str());
}
// fullAtomic decides whether the real file is nuked up front; it has no bearing on the .tmp, which
// both paths open with the same FILE_O_WRITE. This is why the remove must not be scoped to it.
void test_stale_tmp_is_not_appended_to_when_not_full_atomic(void)
{
writeRaw(kTmp, "STALESTALESTALE");
saveThrough(false, "NEW");
TEST_ASSERT_EQUAL_STRING("NEW", readAll(kFile).c_str());
}
// A stale .tmp must not survive the save either, or the next one inherits it again.
void test_completed_save_leaves_no_tmp(void)
{
writeRaw(kTmp, "STALE");
saveThrough(true, "NEW");
TEST_ASSERT_FALSE(FSCom.exists(kTmp));
}
// The rename is an overwrite, not an append onto the previous generation of the real file.
void test_save_replaces_previous_contents(void)
{
writeRaw(kFile, "OLDOLDOLDOLDOLD");
saveThrough(true, "NEW");
TEST_ASSERT_EQUAL_STRING("NEW", readAll(kFile).c_str());
}
// The premise of the suite, asserted rather than assumed: this host truncates on FILE_O_WRITE.
// Portduino only, since it is false by design on the append-on-write backends the fix is aimed at.
#ifdef ARCH_PORTDUINO
void test_write_open_truncates_on_this_host(void)
{
writeRaw(kFile, "AAAAAAAA");
writeRaw(kFile, "B");
TEST_ASSERT_EQUAL_STRING("B", readAll(kFile).c_str());
}
#endif
#endif // FSCom
void setup()
{
initializeTestEnvironment();
UNITY_BEGIN();
#ifdef FSCom
RUN_TEST(test_stale_tmp_is_not_appended_to);
RUN_TEST(test_stale_tmp_is_not_appended_to_when_not_full_atomic);
RUN_TEST(test_completed_save_leaves_no_tmp);
RUN_TEST(test_save_replaces_previous_contents);
#ifdef ARCH_PORTDUINO
RUN_TEST(test_write_open_truncates_on_this_host);
#endif
#endif
exit(UNITY_END());
}
void loop() {}