fix: protocol safety hardening#49
Merged
Merged
Conversation
Add input validation, exception safety, and message size limits to protect against untrusted network input: - Validate array size before element access in Protocol::handleMessage() for all 9 message types - Catch type mismatch exceptions in Protocol::handleMessage() - Add try-catch safety net in BaseNode::handleMessage() for json and std exceptions - Use non-throwing json::parse for all wire formats (JSON/BSON/MSGPACK/CBOR) - Add configurable max message size (default 64MB) to MessageConverter - Add BaseNode::setMaxMessageSize() forwarding method
Resolve data races in three core classes that claimed thread safety but had unprotected access paths. UniqueIdObjectStorage: - Replace dual-mutex with single mutex guarding all state - Fix get() having no lock, remove() TOCTOU, add() size check outside lock BaseNode: - Add mutex protecting m_writeFunc and m_converter - Use copy-and-call in emitWrite() to avoid deadlocks - Log errors outside lock to prevent ABBA deadlock LoggerBase: - Add mutex protecting m_logFunc and m_Loglevel - Use copy-and-call in emitLog() to avoid deadlocks Add concurrent access tests for all three classes.
ClientRegistry::setNode() assigned nodeId to a local copy after it was already inserted into the map, silently discarding the value. Move assignment before insertion. Both ClientRegistry::registerNode() and RemoteRegistry::registerNode() continued to call add() after detecting an expired node weak_ptr. Return invalidId immediately on expiration instead.
ClientNode::handleError() now removes the pending invoke callback and notifies the caller when receiving an Error for Invoke or InvokeReply message types. This prevents unbounded memory growth from unanswered invocations. UniqueIdObjectStorage gains purgeExpired() to remove stale weak_ptrs. add() lazily purges when at capacity before rejecting, reclaiming slots from expired entries.
Add two new CI jobs on ubuntu-latest that build in Debug mode with -fsanitize=thread and -fsanitize=address respectively. Both validated locally with zero warnings.
Use cmake_minimum_required(VERSION 3.14...3.31) so that CMake 4.x on macOS CI runners does not reject fetched dependencies that declare older cmake_minimum_required versions.
Upgrade fetched nlohmann_json from v3.10.5 to v3.11.3 to fix CMake 4.x compatibility on macOS CI runners. The old version declared cmake_minimum_required(VERSION 3.1) which modern CMake rejects. Aligns with template-cpp17 dependency version. Also disable fail-fast in the build matrix so all platforms run independently.
Enable extra warnings for tests only to catch issues early without enforcing them on library consumers: GCC/Clang: -Wshadow, -Wconversion, -Wsign-conversion, -Wnon-virtual-dtor, -Wold-style-cast, -Wcast-align, -Wnull-dereference, -Wdouble-promotion, -Wformat=2, -Wimplicit-fallthrough, plus GCC-specific flags. MSVC: additional /wNNNNN warnings enabled. Fix all warnings surfaced by the new flags across test files and one library header (int vs size_t in emitLog).
Define version in root CMakeLists.txt via project(VERSION)
and reference it with ${PROJECT_VERSION} in src/CMakeLists.txt
instead of a hardcoded string.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
exception safety, configurable max message size)
node handling
Details
Addresses 11 findings from a functional safety and network protocol
investigation. All fixes are wire-protocol-compatible — no changes
to message formats or cross-language behavior. No public API signature
changes (only additions:
setMaxMessageSize,purgeExpired).16 files changed, +1010 / -74 lines, 540 test assertions (was 376).
Test plan