Fix SIGSEGV on ROS 2 Rolling caused by rosidl sequence ABI change#1480
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the JS message code generator template to account for a ROS 2 Rolling rosidl sequence ABI change that can otherwise cause memory layout mismatches (and SIGSEGV) when handling primitive sequences.
Changes:
- Extends the generated
RefStructArray(sequence) struct for primitive base types with two additional boolean fields on ROS 2 Rolling and later. - Adds a runtime ROS distro check in the generated message modules to conditionally include these fields.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| isPrimitivePackage(spec.baseType) | ||
| ? ` ...require('../../lib/distro.js').getDistroId() >= require('../../lib/distro.js').getDistroId('rolling') | ||
| ? { is_rosidl_buffer: ref.types.bool, owns_rosidl_buffer: ref.types.bool } | ||
| : {}` |
There was a problem hiding this comment.
The generated code inlines require('../../lib/distro.js') twice inside an object spread/ternary. Since this runs in every generated message module, it’s more maintainable (and slightly cheaper) to require the module once (e.g., const DistroUtils = ...) and compare against DistroUtils.DistroId.ROLLING (or a single getDistroId('rolling')) rather than repeating the call in the expression.
| ${ | ||
| isPrimitivePackage(spec.baseType) | ||
| ? ` ...require('../../lib/distro.js').getDistroId() >= require('../../lib/distro.js').getDistroId('rolling') | ||
| ? { is_rosidl_buffer: ref.types.bool, owns_rosidl_buffer: ref.types.bool } | ||
| : {}` | ||
| : '' |
There was a problem hiding this comment.
This change gates struct layout on the runtime distro. Please add a regression test that validates the generated output for a primitive sequence includes is_rosidl_buffer/owns_rosidl_buffer when ROS_DISTRO=rolling (and omits them for earlier distros), so future template edits don’t silently reintroduce the Rolling ABI mismatch.
|
coverage: 85.411%. remained the same — minggangw:fix-rolling-crash-caused-rosidl-abi-change into RobotWebTools:develop |
ROS 2 Rolling merged ros2/rosidl#941 and ros2/rosidl#942 as part of the native buffer feature. The ROSIDL_RUNTIME_C__PRIMITIVE_SEQUENCE macro now emits two extra boolean fields (is_rosidl_buffer, owns_rosidl_buffer) in every primitive sequence struct, growing them from 24 to 32 bytes. Non-primitive sequences (e.g. Parameter__Sequence) are unchanged at 24 bytes.
The rclnodejs code generator produces ref-struct-di definitions for these sequence types. Because the generated layout lacked the new fields, every message containing a primitive sequence had misaligned field offsets when serialized, causing Fast DDS to segfault in get_serialized_size_* during rcl_publish.
Changes:
rosidl_gen/templates/message-template.js — Detect Rolling+ at generation time via DistroUtils.getDistroId() and compute needsRosidlBufferFields once per message. Conditionally emit is_rosidl_buffer and owns_rosidl_buffer in the generated sequence struct only for primitive-package types (Bool, Byte, Int8, …, Float64, String) on Rolling+. Generated files get clean, static struct definitions with no runtime require() or ternary.
.github/workflows/linux-x64-build-and-test.yml — Add rosidl_buffer_py and pybind11 to rosdep --skip-keys for the Rolling nightly install, since these new packages from the native buffer feature have unresolvable rosdep keys in the tarball context.
Verified: Rolling: test-parameters (33), test-parameter-service (9), test-action-client (19) — all pass. Jazzy: test-parameters (33), test-parameter-service (9), test-action-client (18+1 pending) — all pass.
Fix: #1481