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 ros2/ros2@4a36f31,

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 #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.

rosidl/rosidl_generator_cpp/resource/msg__struct.hpp.em

Lines 238 to 242 in 0ac4130

	@[for member in message.structure.members]@
	using _@(member.name)_type =
	@(msg_type_to_cpp(member.type));
	_@(member.name)_type @(member.name);
	@[end for]@

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.

rosidl/rosidl_generator_cpp/resource/msg__traits.hpp.em

Lines 265 to 287 in 0ac4130

	@{
	bounded_template_string = 'true'
	bounded_template_strings = set()
	for member in message.structure.members:
	type_ = member.type
	if isinstance(type_, UnboundedSequence):
	bounded_template_string = 'false'
	break
	if isinstance(type_, (Array, BoundedSequence)):
	type_ = type_.value_type
	if isinstance(type_, AbstractGenericString) and not type_.has_maximum_size():
	bounded_template_string = 'false'
	break
	if isinstance(type_, NamespacedType):
	typename = '::'.join(type_.namespaced_name())
	bounded_template_strings.add(f'has_bounded_size<{typename}>::value')
	else:
	if bounded_template_strings:
	bounded_template_string = ' && '.join(sorted(bounded_template_strings))
	}@
	template<>
	struct has_bounded_size<@(message_typename)>
	: std::integral_constant<bool, @(bounded_template_string)> {};

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, 😆 )