Skip to content

picoparams: parse every SetParameters entry, refuse bad input - #26

Merged
g4sp3r merged 1 commit into
Pico-ROS:masterfrom
rosterloh:fix/picoparams-set-parsing
Nov 5, 2026
Merged

g4sp3r merged 1 commit into
Pico-ROS:masterfrom
rosterloh:fix/picoparams-set-parsing

Conversation

@rosterloh

Copy link
Copy Markdown
Contributor

picoparams.c mis-decodes SetParameters requests with more than one entry, and trusts several wire-supplied lengths. Found while running a Pico-ROS parameter server against rmw_zenoh_cpp on ESP32 boards. Independent of #25 (different file), and merges cleanly with it in either order.

Decode every entry of a SetParameters request

deserialize_ros_ParameterValue() returned straight after the scalar it was asked for, leaving string_value and the five array lengths unread. The second Parameter was therefore decoded from the middle of the first and came back as "Parameter not found": setting x and z in one call lost z. All ten fields are now read. Array contents are still not skipped, so array-typed values remain one per request.

Refuse bad input in set_params_value()

  • It looped on the parameter count from the wire, up to 2^32 times on the zenoh read task for a 12-byte request.
  • It reused the previous name when one failed to decode.
  • It skipped an unknown name's value, misaligning every later entry.
  • It could act on a value cut short.

It now stops at the first undecodable entry, reads each value before looking up its name, and writes the result count last. A request cut short gets one result per entry it actually carried, rather than the count it claimed.

params_server_handler() and set_parameters_atomically

  • request_size - 4 was computed unchecked, so an empty request produced a buffer length that wrapped the address space. It now sends no reply.
  • set_parameters_atomically replied with a bare CDR header, which clients fail to decode. It now refuses with a well-formed SetParametersResult.

Well-formed single-parameter requests decode exactly as before.

Testing

🤖 Generated with Claude Code

deserialize_ros_ParameterValue() returned straight after the scalar it was
asked for, leaving string_value and the five array lengths unread, so the
second Parameter of a SetParameters request was decoded from the middle of
the first: it read as "Parameter not found". A set of x and z in one call
lost z. All ten fields are now read (array contents are still not skipped,
so array-typed values remain one-per-request).

set_params_value() trusted the wire's parameter count, looping up to 2^32
times on the zenoh read task for a 12-byte request; reused the previous
name when one failed to decode; skipped an unknown name's value, misaligning
every later entry; and could act on a value cut short. It now stops at the
first undecodable entry, reads each value before looking up its name, and
writes the result count last, so a request cut short gets one result per
entry it carried rather than the count it claimed.

params_server_handler() computed request_size - 4 unchecked, so an empty
request made a buffer that wrapped the address space; it now sends no reply.
set_parameters_atomically replied with a bare CDR header, which clients fail
to decode; it now refuses with a well-formed SetParametersResult.

Signed-off-by: Richard Osterloh <richard.osterloh@gmail.com>
@g4sp3r
g4sp3r merged commit 60bb6c4 into Pico-ROS:master Nov 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants