Harden core subsystems across ECS, logging, networking, I/O#488
Merged
Conversation
Deep-scan audit surfaced a cluster of defensive-programming gaps that could turn a misbehaving peer or corrupt asset into a crash, data leak, or memory exhaustion. This commit fixes the verified ones. - ECSystems.cpp: null-check BehaviorTree::As<>() before Tick() so a wrong-type handle no longer dereferences nullptr. - JobSystem.h: wrap worker task invocation in try/catch so a throwing job cannot silently kill a worker thread. - Logger: cap async message queue at 100k entries with drop-oldest- low-severity policy; prevents runaway producer from exhausting memory. - NetworkManager: bound incoming/outgoing queues at 4096 messages so a flood of packets cannot grow queues without limit. - SaveSystem: reject strings >65535 chars before writing instead of silently truncating to uint16 length prefix (which corrupted saves). - VirtualFileSystem: reject path traversal / absolute paths by returning empty string instead of silently substituting the sandbox root. - ModSystem: skip symlinked mod directories so a crafted mods/ entry cannot redirect loading outside the intended sandbox. Agent scan produced ~140 findings; the majority were false positives (existing guards already in place). The 9 real issues above were verified line-by-line before fixing. Build + full test suite pass.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ae8d99b37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+451
to
+454
| if (m_outgoingQueue.size() >= kMaxQueuedMessages) | ||
| { | ||
| m_droppedOutgoingMessages.fetch_add(1, std::memory_order_relaxed); | ||
| return; |
There was a problem hiding this comment.
SendMessage now drops packets once m_outgoingQueue reaches kMaxQueuedMessages, but this path runs after assigning a reliable sequence number and before the message is ever tracked for retransmission. In a flood scenario, dropping a ReliableOrdered message here creates a sequence gap that the receiver cannot recover from (it buffers later ordered packets until the missing sequence arrives in NetworkManager::Update/FlushOrderedBuffer), so ordered delivery can stall indefinitely instead of degrading gracefully.
Useful? React with 👍 / 👎.
Contributor
❌ CI Error ReportFailed jobs: check-format, clang-tidy, linux-clang-Release, macos-Debug, macos-Release, windows-vs2022-Release Build Errors
Other errors (2)Full error output
|
| Test | Jobs |
|---|---|
| [18:22:25.127] [TID:7360] [WARN ] [Network ] RCON unknown command: nonexistent_cmd (DedicatedServer.cpp:573) | windows-vs2022-Release |
| [18:22:25.141] [TID:7360] [WARN ] [Network ] NetBuffer::ReadUint8 — buffer overrun at pos 1 (size=1) (NetworkBuffer.c... | windows-vs2022-Release |
| [18:22:25.160] [TID:7360] [WARN ] [AI ] BuildNavMeshWithRecast: empty geometry (RecastDetourBackend.cpp:35) | windows-vs2022-Release |
| [18:22:25.160] [TID:7360] [WARN ] [AI ] NavMeshBuilder: Recast build failed, falling back to triangle-soup builder (N... | windows-vs2022-Release |
| [18:22:25.160] [TID:7360] [WARN ] [Audio ] XAudio2 backend requested but no AudioEngine provided, falling back to Nul... | windows-vs2022-Release |
| [ [18:22:25.162] [TID:7360] [WARN ] [Graphics ] No graphics backend available — falling back to NullRHIDevice (headle... | windows-vs2022-Release |
| [18:22:25.162] [TID:7360] [WARN ] [Graphics ] No graphics backend available — falling back to NullRHIDevice (headless... | windows-vs2022-Release |
| [18:22:25.166] [TID:7360] [WARN ] [Editor ] Cannot host: userName is empty. (CollaborativeEditSession.cpp:454) | windows-vs2022-Release |
| [18:22:25.166] [TID:7360] [WARN ] [Editor ] Already connected. (CollaborativeEditSession.cpp:459) | windows-vs2022-Release |
| [18:22:25.169] [TID:7360] [WARN ] [Editor ] BroadcastEdit rejected: nodeId is empty. (CollaborativeEditSession.cpp:1108) | windows-vs2022-Release |
| [18:22:25.169] [TID:7360] [WARN ] [Editor ] BroadcastEdit rejected: sourceEditor is not set. (CollaborativeEditSessio... | windows-vs2022-Release |
| [18:22:25.170] [TID:7360] [WARN ] [Editor ] SetLocalSelection rejected: nodeId exceeds 255 chars (length=300). (Colla... | windows-vs2022-Release |
| [18:22:30.736] [TID:7360] [WARN ] [Network ] Connection rejected for pending client 5: server full (4/4) (NetworkConn... | windows-vs2022-Release |
| [18:22:30.738] [TID:7360] [WARN ] [Network ] Invalid packet magic 0x6A0E5CB3 (expected 0x5350524B) (NetworkManager.cp... | windows-vs2022-Release |
| [18:22:30.738] [TID:7360] [WARN ] [Network ] Invalid packet magic 0xC6BB6E5B (expected 0x5350524B) (NetworkManager.cp... | windows-vs2022-Release |
Compiler Warnings (6)
../../../ThirdParty/SDL2/src/joystick/hidapi/SDL_hidapi_steam.c:165:9: warning: 'printf' macro redefined [-Wmacro-redefined] [linux-clang-Release]
../SparkEngine/Source/Engine/Animation/BlendSpace.cpp:313:16: warning: variable 'validTriCount' set but not used [-Wunused-but-set-variable] [linux-clang-Release]
../SparkEngine/Source/Physics/PhysicsBodyImpl.cpp:270:23: warning: comparing floating point with == or != is unsafe [-Wfloat-equal] [linux-clang-Release]
../SparkEngine/Source/Physics/PhysicsSystem.cpp:401:12: warning: use of old-style cast [-Wold-style-cast] [linux-clang-Release]
../../Tests/TestReliableChannel.cpp:390:9: warning: variable 'retransmitCount' set but not used [-Wunused-but-set-variable] [linux-clang-Release]
../../Tests/TestSparkError.cpp:34:5: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result] [linux-clang-Release]
Updated: 2026-04-17T18:30:48Z — this comment is updated in-place, not duplicated.
CI on the previous commit (3ae8d99) surfaced 12 build errors across linux-clang-Debug, linux-gcc-Debug, linux-asan/tsan, linux-msan, coverage, and a Windows-Release test crash. Not all of them were introduced by that commit — most were pre-existing — but fixing them here is easier than gating each job. Build fixes - SparkEngineLinux.cpp: GraphicsEngine has no IsHeadless(); route the check through GetRHIBridge()->IsHeadless() instead. This call site was breaking coverage + gcc-Debug + asan + tsan. - Daemon code: std::expected/std::unexpected are unavailable on the clang-Debug toolchain used in CI. Added SparkEngine/Source/Utils/ Expected.h which aliases std::expected when __cpp_lib_expected is satisfied, otherwise provides a small polyfill with the subset of operations this codebase uses (has_value, operator bool, error, value, operator*, operator->). DaemonServer, DaemonClient, ShaderServiceClient, AssetServiceClient all now take Spark::Daemon::Expected / Unexpected. - Tests/TestDaemonConcurrent, TestDaemonLRU, TestDaemonCodexFixes: add <unistd.h> so getpid() / unlink() are visible under libc++ used by the msan job. Test guards - TestNetworkStack::NetworkStack_UDPInitializeSucceeds: avoid chaining GetTransport()->GetTransportName() after EXPECT_TRUE(transport) — the test framework's EXPECT_TRUE doesn't short-circuit, so a null transport (seen on Windows Release) was crashing the whole suite. Now returns early after the null-check. Address Codex review (PR #488 comment on 3ae8d99) - NetworkManager::SendMessage / ProcessIncoming: the previous bounded- queue fix dropped messages of any channel once the queue filled, which creates an unrecoverable sequence gap for ReliableOrdered traffic. Back-pressure now only drops Unreliable messages; reliable traffic still queues, with the assumption the receiver's ack/retry path will recover. Sequence numbers are also assigned *after* the drop check so we don't burn a sequence on a dropped packet. Local: linux-gcc-release, linux-gcc-debug, linux-clang-debug all build clean and the test suite passes on all three.
Contributor
Code Coverage (GCC + lcov)Per-Subsystem Coverage
Total: 50.3% (27446/54577 lines) |
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
2 participants
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.
Deep-scan audit surfaced a cluster of defensive-programming gaps that could turn a misbehaving peer or corrupt asset into a crash, data leak, or memory exhaustion. This commit fixes the verified ones.
Agent scan produced ~140 findings; the majority were false positives (existing guards already in place). The 9 real issues above were verified line-by-line before fixing. Build + full test suite pass.