Port: Stream json garbage infinite loop and log json stream error#2135
Open
SatheeshkumarG15 wants to merge 8 commits into
Open
Port: Stream json garbage infinite loop and log json stream error#2135SatheeshkumarG15 wants to merge 8 commits into
SatheeshkumarG15 wants to merge 8 commits into
Conversation
Fixes #1963. Two related bugs caused an infinite spin when a TCP stream carried non-JSON data (garbage bytes) followed by a valid JSON message. Bug 1 - IsNullValue() off-by-one (JSON.h) The guard 'loaded + 1 >= maxLength' fired one iteration too early, returning UNKNOWN without consuming the last byte of the input slice. On the next call the same byte was presented again, producing another UNKNOWN, and so on forever. Fix: change the guard to 'loaded >= maxLength' so the last available byte is consumed before the function signals that more data is needed. Bug 2 - ReceiveData() no-progress loop (StreamJSON.h) The do/while loop in ReceiveData() had no guard against Deserialize() returning 0 (no bytes consumed). If the parser stalled, 'handled' never advanced past 'receivedSize' and the loop spun indefinitely. Fix: capture the return value of each Deserialize() call and break the loop when it returns 0. A reproducer test is added in Tests/streamjson-garbage/. It opens a TCP server using StreamJSONType, connects a raw client, sends garbage bytes followed by a valid JSONRPC message, and asserts that: - the server does not hang - the valid JSON object after the garbage is successfully parsed - the connection closes cleanly within 3 seconds
- Guard POSIX-specific includes and client socket code with #ifdef __POSIX__ to prevent build failures on Windows. - Restrict CMake target to non-Windows platforms with if(NOT CMAKE_SYSTEM_NAME STREQUAL "Windows"). - Make JSONFactory a static member of JSONServer so the StreamJSONType base class receives a reference to a fully-constructed object, eliminating the undefined-behavior reference-before-construction.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Backports fixes for StreamJSON handling of garbage/non-JSON bytes on TCP streams by preventing no-progress spins during deserialization, correcting an off-by-one in JSON null parsing, and adding error logging plus a standalone reproducer test.
Changes:
- Fix off-by-one in
Core::JSON::IsNullValue()to ensure the last available byte is consumed before returningUNKNOWN. - Prevent infinite looping in
StreamJSONType::ReceiveData()when deserialization consumes 0 bytes; add throttled parse-error logging and reset logic. - Add an optional standalone test executable (
StreamJSONGarbageTest) and CMake option to reproduce the original hang scenario.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/streamjson-garbage/Module.h | Test module definitions for the standalone reproducer executable. |
| Tests/streamjson-garbage/Module.cpp | Declares the test module build reference. |
| Tests/streamjson-garbage/GarbageTest.cpp | Standalone reproducer that sends garbage + valid JSON and verifies the server doesn’t hang. |
| Tests/streamjson-garbage/CMakeLists.txt | Adds build/install rules for the standalone reproducer executable. |
| Tests/CMakeLists.txt | Introduces STREAMJSON_GARBAGE_TEST option to include the new test directory. |
| Source/core/StreamJSON.h | Adds error-aware deserialization, throttled logging, and a no-progress guard in ReceiveData(). |
| Source/core/JSON.h | Fixes IsNullValue() guard condition to avoid stalling on 1-byte slices. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+12
to
+17
| target_compile_options(StreamJSONGarbageTest PRIVATE -pthread) | ||
| target_link_options(StreamJSONGarbageTest PRIVATE -pthread) | ||
|
|
||
| target_link_libraries(StreamJSONGarbageTest | ||
| PRIVATE | ||
| ${NAMESPACE}Core |
Comment on lines
+1
to
+5
| if(NOT CMAKE_SYSTEM_NAME STREQUAL "Windows") | ||
| add_executable(StreamJSONGarbageTest | ||
| Module.cpp | ||
| GarbageTest.cpp | ||
| ) |
Comment on lines
+157
to
+159
| CC_SYSLOG("StreamJSONType failed: %s", ErrorDisplayMessage(error.Value()).c_str()); | ||
| } else { | ||
| CC_SYSLOG("StreamJSONType failed (errors after this one will not be reported until correct message is received again): %s", ErrorDisplayMessage(error.Value()).c_str()); |
Comment on lines
+94
to
+97
| // NOTE: _factory is declared AFTER the StreamJSONType base so it is | ||
| // technically initialised after the base-class ctor runs. The base stores | ||
| // the reference for later use (not in the ctor body), so this is safe in | ||
| // practice and matches the pattern used by the existing unit tests. |
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
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.
This PR combines the backports of #2129 and #2133