Length of size limited string is not checked

[erikschuitema]

Bug report

Required Info:

• Operating System:
  • Ubuntu 20.04
• Installation type:
  • Binaries
• Version or commit hash:
  • ros-foxy-rclcpp/focal,now 2.4.0-1focal.20211014.182342 amd64
• DDS implementation:
  • Fast-RTPS
• Client library (if applicable):
  • rclcpp

Steps to reproduce issue

Create a message type (StringLengthTest.msg) with size limited string field:

string<=10 size_limited_string

Publish a message with a lengthier string:

auto node = std::make_shared<rclcpp::Node>("string_length_test_publisher");
auto publisher = node->create_publisher<msg_tester_interfaces::msg::StringLengthTest>("oversized", 10);
auto message = msg_tester_interfaces::msg::StringLengthTest();
message.size_limited_string.resize(20, '.');
publisher->publish(message);
rclcpp::spin_some(node);

Expected behavior

Exception of type std::length_error.

Actual behavior

Message is published and received in full by an rclcpp subscriber, i.e., no error and no truncation.

Side note: an rclpy subscriber does throw an error:

AssertionError: The 'size_limited_string' field must be string value not longer than 10

[fujitatomoya]

confirmed with mainline https://github.com/ros2/ros2/commit/4a36f319afcc208deae3c373493ef6d9feda1324,

the following is reproducible colcon workspace project, https://github.com/fujitatomoya/ros2_test_prover.

# cd <your_concol_workspace>/src/ros2
# git clone https://github.com/fujitatomoya/ros2_test_prover
# cd <your_concol_workspace>
# colcon build --symlink-install --packages-select prover_rclcpp prover_rclpy prover_interfaces
# ros2 run prover_rclcpp rclcpp_1827

and we can not echo the topic since rclpy cannot convert it into Python object...

# ros2 topic echo /oversized
make_tuple(): unable to convert argument of type 'object' to Python object

[aprotyas]

I don't know how accurate this comment is, but from rosidl_generator_cpp/test/test_interfaces.cpp

// TODO(mikaelarguedas) reenable this test when bounded strings enforce length
TEST(Test_messages, DISABLED_Test_bounded_strings) {
  rosidl_generator_cpp::msg::Strings message;
  TEST_STRING_FIELD_ASSIGNMENT(message, bounded_string_value, "", "Deep into")
  std::string tooLongString = std::string("Too long string");
  message.bounded_string_value = tooLongString;
  tooLongString.resize(BOUNDED_STRING_LENGTH);
  ASSERT_STREQ(tooLongString.c_str(), message.string_value.c_str());
}

It seems like bounded length string support has not been fully implemented? I see the difficulty of course, since std::string can't easily be constrained in length. That comment was introduced in https://github.com/ros2/rosidl/pull/179 for added context.

[Barry-Xu-2018]

As aprotyas said, bounded length string isn't supported in current implementation.
No matter whether you add length limitation, generated codes are almost the same.

https://github.com/ros2/rosidl/blob/0ac413097fd1450e83e0ca8a280d097fe38b32e7/rosidl_generator_cpp/resource/msg__struct.hpp.em#L238-L242

Generated code as below

  using _size_limited_string_type =
    std::basic_string<char, std::char_traits<char>, typename std::allocator_traits<ContainerAllocator>::template rebind_alloc<char>>;
  _size_limited_string_type size_limited_string;

size_limited_string is type std::basic_string. User can resize it arbitrarily.
In order to support length checking, a new string class seems to be necessary.

For current implementation, user only can know whether string is bounded.

https://github.com/ros2/rosidl/blob/0ac413097fd1450e83e0ca8a280d097fe38b32e7/rosidl_generator_cpp/resource/msg__traits.hpp.em#L265-L287

Generated code as below

template<>
struct has_bounded_size<interfaces_bug::msg::StringLengthTest>
  : std::integral_constant<bool, true> {};

[aprotyas]

I think this issue should be transferred to ros2/rosidl - that repository houses the packages which generate/support message structures/traits.

[hidmic]

In order to support length checking, a new string class seems to be necessary.

Indeed, a bounded_basic_string with checks upon construction and assignment would do. We should make it implicitly convertible to a basic_string to not break downstream code.

[iuhilnehc-ynos]

If no one is working on this issue, I would like to contribute to it.

[aprotyas]

If no one is working on this issue, I would like to contribute to it.

This was on my backlog but feel free to proceed!

[iuhilnehc-ynos]

This was on my backlog but feel free to proceed!

Oh, it seems better if you (@aprotyas) fix this issue. (To review my PR might cost you much more time, :laughing: )