fix(parser_ros): bound-check CDR byte-sequence length to prevent OOB read#183
Merged
Merged
Conversation
…read NanoCDR_Deserializer::deserializeByteSequence() trusts the wire-declared array length and does not clamp it to the buffer (unlike the ROS1 deserializer and NanoCDR's own string reader). A truncated or corrupt message therefore yields a span extending past the payload; reading it downstream is an out-of-bounds access (verified SIGSEGV decoding a truncated sensor_msgs/Image in a 2D image view). Route every bulk uint8[] read through a new RosParser::readByteSequence() that validates the declared length against the bytes remaining and throws on overrun, matching the ROS1 path. Since a throw across the noexcept C-ABI trampoline would call std::terminate, guard the scalar void-handler route in wrapVoidHandler() (the object handlers already carry their own try/catch); this also closes the pre-existing deserializeString overrun on that route. Add regression tests for a truncated Image (object route rejected) and a truncated scalar message (scalar route fails cleanly, no terminate).
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
RosParser::readByteSequence(), a single choke point that validates a CDRuint8[]/sequence<uint8>length against the bytes remaining in the message and throws on overrun (mirroring the ROS1 deserializer).wrapVoidHandler()with try/catch so a decode throw becomes anExpectederror instead ofstd::terminateacross thenoexceptC-ABI trampoline (the object handlers already have their own try/catch).sensor_msgs/Image(object route) and a truncated scalar message (scalar route).Why
NanoCDR_Deserializer::deserializeByteSequence()trusts the wire-declared array length and does not clamp it to the buffer — unlike the ROS1 deserializer and NanoCDR's own string reader, both of which bounds-check. A truncated or corrupt message (e.g. a partially-written bag, or asensor_msgs/msg/Imagewhosedata[]declares more bytes than are present) therefore produced ansdk::Imagewhose data span extended far past the payload. The 2D image viewer's per-row copy read off the end of the buffer → heap out-of-bounds read / SIGSEGV (reproduced on a 64-bit desktop build, not just 32-bit). The defect lives in the shared byte-sequence primitive, so every builtin-object handler that reads a bulkdata[]shared the same exposure;sensor_msgs/msg/Imagewas the most common trigger.rosx_introspectionis a pinned upstream release, so the guard is added on the consumer side inparser_ros.Testing
ctest --test-dir build -R ros_parser(or runros_parser_test): 72 tests pass, includingImageWithTruncatedDataIsRejectedandTruncatedScalarMessageDoesNotTerminate.sensor_msgs/msg/Imageand open the topic in a 2D image view — previously crashed, now the frame is rejected cleanly.-Wall -Wextra -Werror; pre-commit (clang-format) passes.