diff --git a/parser_ros/ros_builtin_object_handlers.cpp b/parser_ros/ros_builtin_object_handlers.cpp index e1f0b2d..ab2569b 100644 --- a/parser_ros/ros_builtin_object_handlers.cpp +++ b/parser_ros/ros_builtin_object_handlers.cpp @@ -115,7 +115,7 @@ PJ::Expected RosParser::parseImage(PJ::Timestamp ts, PJ:: deserializer_->deserializeString(encoding); const uint8_t is_be = readU8(*deserializer_); const uint32_t step = deserializer_->deserializeUInt32(); - const auto data_span = deserializer_->deserializeByteSequence(); + const auto data_span = readByteSequence(); auto it = kRosImageBytesPerPixel().find(encoding); if (it == kRosImageBytesPerPixel().end()) { @@ -170,7 +170,7 @@ PJ::Expected RosParser::parseCompressedImage(PJ::Timestam std::string format; deserializer_->deserializeString(format); - const auto data_span = deserializer_->deserializeByteSequence(); + const auto data_span = readByteSequence(); const uint8_t* src = data_span.data(); const uint32_t data_len = static_cast(data_span.size()); @@ -334,7 +334,7 @@ PJ::Expected RosParser::parseCompressedVideo(PJ::Timestam std::string frame_id; deserializer_->deserializeString(frame_id); - const auto data_span = deserializer_->deserializeByteSequence(); + const auto data_span = readByteSequence(); std::string format; deserializer_->deserializeString(format); @@ -408,7 +408,7 @@ PJ::Expected RosParser::parsePointCloud(PJ::Timestamp ts, const uint8_t is_be = readU8(*deserializer_); const uint32_t point_step = deserializer_->deserializeUInt32(); const uint32_t row_step = deserializer_->deserializeUInt32(); - const auto data_span = deserializer_->deserializeByteSequence(); + const auto data_span = readByteSequence(); // is_dense follows data[]. deserializeByteSequence advanced the cursor // past the body, so the next byte read is is_dense itself. @@ -574,7 +574,7 @@ PJ::Expected RosParser::parseFoxgloveCompressedPointCloud (void)deserializer_->deserialize(RosMsgParser::FLOAT64); } - const auto data_span = deserializer_->deserializeByteSequence(); + const auto data_span = readByteSequence(); std::string format; deserializer_->deserializeString(format); @@ -639,7 +639,7 @@ PJ::Expected RosParser::parseCompressedPointCloud2( (void)readU8(*deserializer_); // is_bigendian (void)deserializer_->deserializeUInt32(); // point_step (void)deserializer_->deserializeUInt32(); // row_step - const auto data_span = deserializer_->deserializeByteSequence(); + const auto data_span = readByteSequence(); (void)readU8(*deserializer_); // is_dense std::string format; @@ -1016,7 +1016,7 @@ PJ::Expected RosParser::parseOccupancyGrid(PJ::Timestamp const double oz = deserializer_->deserialize(RosMsgParser::FLOAT64).convert(); const double ow = deserializer_->deserialize(RosMsgParser::FLOAT64).convert(); - const auto data_span = deserializer_->deserializeByteSequence(); + const auto data_span = readByteSequence(); PJ::sdk::OccupancyGrid grid; grid.timestamp_ns = current_timestamp_; @@ -1066,7 +1066,7 @@ PJ::Expected RosParser::parseOccupancyGridUpdate( const int32_t y = deserializer_->deserialize(RosMsgParser::INT32).convert(); const uint32_t width = deserializer_->deserializeUInt32(); const uint32_t height = deserializer_->deserializeUInt32(); - const auto data_span = deserializer_->deserializeByteSequence(); + const auto data_span = readByteSequence(); const size_t expected = static_cast(width) * static_cast(height); if (data_span.size() < expected) { diff --git a/parser_ros/ros_marker_handlers.cpp b/parser_ros/ros_marker_handlers.cpp index ea14f7e..f30999e 100644 --- a/parser_ros/ros_marker_handlers.cpp +++ b/parser_ros/ros_marker_handlers.cpp @@ -163,7 +163,7 @@ void RosParser::decodeOneMarker(PJ::sdk::SceneEntities& out) { deserializer_->deserializeString(texture_frame); std::string texture_format; deserializer_->deserializeString(texture_format); - (void)deserializer_->deserializeByteSequence(); // texture.data + (void)readByteSequence(); // texture.data const uint32_t num_uv = deserializer_->deserializeUInt32(); if (num_uv > kMaxMarkerVertices) { throw std::runtime_error("Marker uv_coordinates[] exceeds sanity cap"); @@ -185,7 +185,7 @@ void RosParser::decodeOneMarker(PJ::sdk::SceneEntities& out) { // The byte sequence must be consumed unconditionally to keep the wire // aligned, but only a MESH_RESOURCE marker uses it — skip the copy of a // potentially multi-MB payload for every other marker type. - const auto bytes = deserializer_->deserializeByteSequence(); // mesh_file.data + const auto bytes = readByteSequence(); // mesh_file.data if (type == marker_type::kMeshResource) { mesh_file_data.assign(bytes.begin(), bytes.end()); } diff --git a/parser_ros/ros_parser.cpp b/parser_ros/ros_parser.cpp index 70187e8..9e91dca 100644 --- a/parser_ros/ros_parser.cpp +++ b/parser_ros/ros_parser.cpp @@ -1,7 +1,9 @@ #include +#include #include #include #include +#include #include #include "ros_parser_internal.hpp" @@ -626,6 +628,21 @@ void RosParser::ensureDeserializer() { } } +RosMsgParser::Span RosParser::readByteSequence() { + // Bytes available at the cursor, captured before the read (the 4-byte length + // prefix and any trailing fields are included, so the bound is conservative: + // it can only over-accept by that slack, never falsely reject a full message). + const size_t available = deserializer_->bytesLeft(); + const auto span = deserializer_->deserializeByteSequence(); + if (available < sizeof(uint32_t) || span.size() > available - sizeof(uint32_t)) { + // The declared length ran past the payload. deserializeByteSequence has already + // advanced the cursor past the end, so both this span AND any field the handler + // reads next would be out of bounds; abort now (caught upstream -> Expected error). + throw std::runtime_error("CDR byte sequence length exceeds message payload (truncated or corrupt message)"); + } + return span; +} + void RosParser::detectSchemaFeatures() { const auto& schema = parser_->getSchema(); const auto& root_fields = schema->root_msg->fields(); diff --git a/parser_ros/ros_parser_internal.hpp b/parser_ros/ros_parser_internal.hpp index 4b4c63d..c6151d6 100644 --- a/parser_ros/ros_parser_internal.hpp +++ b/parser_ros/ros_parser_internal.hpp @@ -254,6 +254,19 @@ class RosParser : public PJ::MessageParserPluginBase { // overrides (e.g. std_msgs/String on a robot_description topic -> RobotDescription). CatalogEntry selectCatalogEntry(const std::string& msg_type) const; void ensureDeserializer(); + // Reads a CDR byte sequence (uint8[] / sequence) and validates its + // wire-declared length against the bytes remaining in the message. NanoCDR's + // byte-sequence reader trusts the declared length and does not clamp it to the + // buffer (unlike the ROS1 deserializer and NanoCDR's own string reader): on a + // truncated/corrupt message it advances the cursor past the end and returns a span + // longer than the payload. Reading that span — or any field the handler reads + // after it, since the cursor is now past the end — is an out-of-bounds access. + // Throws std::runtime_error on overrun (matching the ROS1 reader) so the decode is + // aborted at once. Every object handler wraps its body in try/catch and the scalar + // void-handler path is guarded in wrapVoidHandler(), so the throw becomes an + // Expected error (no frame, no crash), never a std::terminate across the C ABI. + // Read all bulk uint8[] fields through this, never the deserializer directly. + RosMsgParser::Span readByteSequence(); void detectSchemaFeatures(); void findQuaternionPrefixes( const RosMsgParser::ROSMessage* msg, const std::string& prefix, const RosMsgParser::RosMessageLibrary& lib); @@ -443,7 +456,15 @@ class RosParser : public PJ::MessageParserPluginBase { named_fields_.clear(); current_timestamp_ = ts; deserializer_->init(RosMsgParser::Span(payload.data(), payload.size())); - (this->*Handler)(); + // The object handlers each carry their own try/catch; the scalar void handlers + // do not, and this wrapper is the C-ABI-facing entry for them. A CDR decode + // throw (e.g. readByteSequence / deserializeString on a truncated message) must + // become an Expected error here, never propagate into the noexcept trampoline. + try { + (this->*Handler)(); + } catch (const std::exception& e) { + return PJ::unexpected(std::string("ROS scalar decode error: ") + e.what()); + } std::vector out; out.reserve(owned_fields_.size()); for (const auto& f : owned_fields_) { diff --git a/parser_ros/ros_specializations.cpp b/parser_ros/ros_specializations.cpp index 5a4ed19..0485ad6 100644 --- a/parser_ros/ros_specializations.cpp +++ b/parser_ros/ros_specializations.cpp @@ -267,8 +267,8 @@ void RosParser::handleDataTamerSnapshot() { uint64_t timestamp = deserializer_->deserialize(RosMsgParser::UINT64).convert(); uint64_t schema_hash = deserializer_->deserialize(RosMsgParser::UINT64).convert(); - auto active_mask = deserializer_->deserializeByteSequence(); - auto payload = deserializer_->deserializeByteSequence(); + auto active_mask = readByteSequence(); + auto payload = readByteSequence(); auto it = g_data_tamer_schemas.find(schema_hash); if (it == g_data_tamer_schemas.end()) { diff --git a/parser_ros/tests/ros_parser_test.cpp b/parser_ros/tests/ros_parser_test.cpp index 9a634d8..177f885 100644 --- a/parser_ros/tests/ros_parser_test.cpp +++ b/parser_ros/tests/ros_parser_test.cpp @@ -779,6 +779,24 @@ TEST(RosParserTest, PoseWithRPY) { EXPECT_NE(findField(f.recorder.rows()[0], "/orientation/pitch"), nullptr); } +// Regression (scalar route): a truncated message whose scalar (void) handler reads +// past the end must fail cleanly, not std::terminate. The void handlers run through +// wrapVoidHandler(), which — unlike the object handlers — is the C-ABI-facing entry +// for the scalar route, so it must catch the CDR decode throw before it crosses the +// noexcept trampoline. Here geometry_msgs/Pose carries only position (3 doubles); +// handlePose throws reading the missing orientation. +TEST(RosParserTest, TruncatedScalarMessageDoesNotTerminate) { + RosParserFixture f; + f.setUp(); + ASSERT_TRUE(f.bindSchema("geometry_msgs/Pose", kPoseDef)); + + auto payload = serializeCdr([](RosMsgParser::NanoCDR_Serializer& enc) { + serializeVector3(enc, 1.0, 2.0, 3.0); // position only; orientation truncated away + }); + + EXPECT_FALSE(f.parse(payload)) << "truncated scalar message must fail cleanly, not terminate"; +} + TEST(RosParserTest, ImuRPY) { RosParserFixture f; f.setUp(); @@ -1565,6 +1583,45 @@ TEST(RosParserTest, ImageObjectCarriesFrameId) { EXPECT_EQ(img->encoding, "mono8"); } +// Regression: a truncated sensor_msgs/Image whose data[] declares more bytes than +// the message carries must be REJECTED, not decoded into a span that runs past the +// payload. NanoCDR's byte-sequence reader does not clamp the declared length to the +// buffer, so readByteSequence() adds the bound check the ROS1 path already has. +// Without the fix, parseImage would emit an Image whose data span extends ~82 MB +// past a 48-byte payload and the 2D viewer's copy would read off the end (SIGSEGV). +TEST(RosParserTest, ImageWithTruncatedDataIsRejected) { + static const char* kImageDef = + "std_msgs/Header header\nuint32 height\nuint32 width\nstring encoding\n" + "uint8 is_bigendian\nuint32 step\nuint8[] data\n" + "================\nMSG: std_msgs/Header\nbuiltin_interfaces/Time stamp\nstring frame_id\n" + "================\nMSG: builtin_interfaces/Time\nint32 sec\nuint32 nanosec\n"; + + RosParserFixture f; + f.setUp(); + ASSERT_TRUE(f.bindSchema("sensor_msgs/Image", kImageDef)); + + // Declare a data[] of step*height bytes but write NONE of them: the message ends + // right after the count. Mirrors a partially-written / corrupt bag. + constexpr uint32_t kHeight = 20000; + constexpr uint32_t kWidth = 4096; + constexpr uint32_t kStep = 4096; // mono8 -> step == width*bpp + auto payload = serializeCdr([](RosMsgParser::NanoCDR_Serializer& enc) { + serializeHeader(enc, 7, 0, "camera_link"); + enc.serializeUInt32(kHeight); + enc.serializeUInt32(kWidth); + enc.serializeString("mono8"); + enc.serialize(RosMsgParser::UINT8, RosMsgParser::Variant(static_cast(0))); // is_bigendian + enc.serializeUInt32(kStep); + enc.serializeUInt32(kStep * kHeight); // data[] count -- but no bytes follow + }); + + auto* base = static_cast(f.handle.context()); + ASSERT_NE(base, nullptr); + const PJ::sdk::PayloadView view{PJ::Span(payload.data(), payload.size()), {}}; + auto rec = base->parseObject(1234, view); + EXPECT_FALSE(rec.has_value()) << "truncated Image must be rejected, not decoded into an out-of-bounds span"; +} + // Regression: ROS Bayer CFA images (bayer_rggb8 and friends) carry one raw mosaic // sample per pixel (1 byte/pixel). parseImage must ACCEPT them and pass the encoding // through verbatim — the viewer demosaics downstream. Previously these were rejected