Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Length of size limited string is not checked #637

Open
erikschuitema opened this issue Nov 22, 2021 · 8 comments · May be fixed by #645
Open

Length of size limited string is not checked #637

erikschuitema opened this issue Nov 22, 2021 · 8 comments · May be fixed by #645
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@erikschuitema
Copy link

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 referenced this issue in fujitatomoya/ros2_test_prover Nov 22, 2021
@fujitatomoya
Length of size limited string is not checked
fabd65b 
  https://github.com/ros2/rclcpp/issues/1827

Signed-off-by: Tomoya Fujita <[email protected]>
@fujitatomoya
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Member

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

@aprotyas aprotyas transferred this issue from ros2/rclcpp Dec 6, 2021
@hidmic hidmic added the bug Something isn't working label Dec 8, 2021
@hidmic
Copy link
Contributor

hidmic commented Dec 8, 2021

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.

@clalancette clalancette added the help wanted Extra attention is needed label Dec 23, 2021
@iuhilnehc-ynos
Copy link
Contributor

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

@aprotyas
Copy link
Member

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
Copy link
Contributor

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

@aprotyas aprotyas linked a pull request Jan 13, 2022 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add bounded string support in C++ ros2/rosidl
7 participants
@clalancette @hidmic @erikschuitema @aprotyas @fujitatomoya @Barry-Xu-2018 @iuhilnehc-ynos