Bound string decoding, echo request identity in service replies, check z_malloc

[rosterloh]

Three robustness fixes found while running Pico-ROS nodes against rmw_zenoh_cpp (Lyrical) on ESP32 boards. Each is its own commit, and the string-bounds fix comes with tests.

1. Bound string and string-sequence decoding (picoserdes.c)

ucdr_deserialize_rstring() advanced the iterator by the length read off the wire without checking it against the buffer. It also returned a pointer whose terminating NUL it never checked. A malformed or hostile peer could therefore make every later read, and any strcmp() on the returned string, run past the received buffer. A length near UINT32_MAX wraps the iterator backwards on 32-bit targets.

The decoder now refuses a zero length, a length beyond the remaining buffer, or a string whose last byte isn't NUL. In each case it sets ub->error and doesn't move the iterator. ucdr_deserialize_sequence_rstring() also bounds its count against max_number and leaves *number at 0 on failure. Well-formed input decodes exactly as before.

2. Echo the request's rmw identity in service replies (picoros.c, picoros.h)

queriable_data_handler() replied with sequence_number = 1 and the server's own GID every time; it never read the request's attachment. rmw_zenoh clients match a reply to its pending request by that sequence number. rclpy, for example, drops a reply with no matching pending request. As a result, a long-lived client gets its first call answered and every later reply dropped, and the call times out. ros2 service call creates a fresh client for each call, which always starts at sequence 1, so it passes and hides the bug.

To reproduce: call a Pico-ROS service twice from one rclpy client. The second call times out.

The fix is a small static-inline helper, picoros_reply_attachment_from_request(). The handler builds each reply's attachment in a stack-local copy of srv->attachment:

• If the request attachment is well-formed (exactly sizeof(rmw_attachment_t) bytes), its sequence number and GID are echoed into the reply.
• Anything else, including no attachment at all, keeps today's values.

srv->attachment itself is never written, so one client's GID can't leak into later replies. time is still the server's own clock. z_query_reply() sends synchronously, so pointing z_bytes_from_static_buf() at the stack copy is safe.

3. Check z_malloc in the receive paths and client init (picoros.c)

The subscriber, queryable and service-reply handlers copied each incoming payload into an unchecked z_malloc() buffer. On a small target, heap exhaustion became a write through NULL inside the zenoh receive task. Now:

• The subscriber and reply handlers drop the sample or reply and log it.
• The queryable handler leaves the request unanswered, so the caller times out rather than the node crashing.
• picoros_service_client_init() returns PICOROS_ERROR if it can't allocate its key buffer.

z_malloc(0) may legitimately return NULL, so an empty service request is not treated as a failure.

Testing

• test_examples_types_serdes passes. It has a new "Malformed String Tests" block with four lying-length strings and one over-capacity string sequence. All five cases fail against the current decoder and pass with fix 1, and the existing round-trip tests are unchanged.
• On macOS with clang, zenoh-pico doesn't build as a library: atomic.c requires GCC or C11. So picoros.c was compile-checked on its own with the project's own flags and -Wall -Wextra. The patch adds no new warnings.
• We carry these patches downstream on top of b739fc7. There they are built into Zephyr firmware for ESP32 boards, with host-side unit checks for the reply-attachment helper. The multi-call scenario in fix 2 has not yet been exercised on hardware against a live rmw_zenohd.

Happy to split this into three PRs if you'd prefer.

🤖 Generated with Claude Code