[Foxy] Handle exception on deserializing ROS message

[suurjaak]

Fixes ros2/rclpy#923.

Minimal code for reproducing error:

import rclpy.serialization, std_msgs.msg
rclpy.serialization.deserialize_message(b"", std_msgs.msg.String)

Result without the fix:

user@host:~$ python3
Python 3.8.10 (default, Sep 28 2021, 16:10:42) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import rclpy.serialization, std_msgs.msg
>>> rclpy.serialization.deserialize_message(b"", std_msgs.msg.String)
terminate called after throwing an instance of 'eprosima::fastcdr::exception::NotEnoughMemoryException'
  what():  Not enough memory in the buffer stream
Aborted (core dumped)
user@host:~$

Result with the fix:

user@host:~$ python3
Python 3.8.10 (default, Sep 28 2021, 16:10:42) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import rclpy.serialization, std_msgs.msg
>>> rclpy.serialization.deserialize_message(b"", std_msgs.msg.String)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/ros/foxy/lib/python3.8/site-packages/rclpy/serialization.py", line 42, in deserialize_message
    return _rclpy.rclpy_deserialize(serialized_message, message_type)
_rclpy.RCLError: Failed to deserialize ROS message
>>>

[fujitatomoya]

@suurjaak i just noticed that we could do backport this?

[suurjaak]

@suurjaak i just noticed that we could do backport this?

Indeed, that looks fully compatible, and more complete.

And looking it over, I noticed a few more things I would change, to make deserialization more robust. The dynamic_cpp version can throw std::runtime_error which would also crash and terminate the process, I believe.

How should I go about this - cherry-pick commits from that pull request into this one?

[fujitatomoya]

How should I go about this - cherry-pick commits from that pull request into this one?

#505 has been merged to master, humble and galactic. So yes, you can cherry-pick the PR.

The dynamic_cpp version can throw std::runtime_error which would also crash and terminate the process, I believe.

#505 seems to address this too. If you have additional fix related, i would create another PR to master then backport it to humble, galactic and foxy.

[suurjaak]

The dynamic_cpp version can throw std::runtime_error which would also crash and terminate the process, I believe.

#505 seems to address this too. If you have additional fix related, i would create another PR to master then backport it to humble, galactic and foxy.

As an example, deserializeROSMessage(Cdr&, void*, void*) calls the overloaded deserializeROSMessage(Cdr&, MembersType*, void*), and the latter throws runtime_error on a number of conditions. But the former only catches fastcdr exceptions.

Does this not look equally crash-inducing? rclpy should not just terminate upon receiving invalid data.

[fujitatomoya]

#505 addresses CDR exception, which i think we can fix original issue for foxy ros2/rclpy#923.

As an example, deserializeROSMessage(Cdr&, void*, void*) calls the overloaded deserializeROSMessage(Cdr&, MembersType*, void*), and the latter throws runtime_error on a number of conditions. But the former only catches fastcdr exceptions.

i am not sure if we can make this happen, but as you pointed out this possibly generates the std::runtime_error.

[suurjaak]

Added #505.

i am not sure if we can make this happen, but as you pointed out this possibly generates the std::runtime_error.

Can you clarify this comment? I am not sure what you meant here.

[fujitatomoya]

i mean i am not sure if we can make it happen. if you are willing to address this, i am happy to review PR too !!! in that case, probably we would want to have another PR, so that we can have individual PR for each problem.

[suurjaak]

Regarding making another pull request - there appears no need, as the current ROS2 master no longer terminates rclpy on a C++ exception, but provides a catchable exception in Python instead.
At least that was the behavior when I ran tests on a locally-built master.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

@MiguelCompany @ivanpauno requesting another review.

[suurjaak]

@fujitatomoya Is there a problem on the buildfarm?

Package 'rmw_fastrtps_dynamic_cpp' specified with --packages-above-and-dependencies was not found

[MiguelCompany]

Package 'rmw_fastrtps_dynamic_cpp' specified with --packages-above-and-dependencies was not found

I think the reason for this is that the CI was launched with use_fastrtps_dynamic: false, and that puts a COLCON_IGNORE on rmw_fastrtps_dynamic_cpp

[MiguelCompany]

LGTM!

[fujitatomoya]

CI(retry)

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

windows warnings and errors are unrelated, just in case retry.

• Windows

[fujitatomoya]

CI: (only CI_USE_FASTRTPS_STATIC)

• Windows

[fujitatomoya]

wrong operation, retry CI: (only CI_USE_FASTRTPS_STATIC)

• Windows

[fujitatomoya]

@clalancette do we need to run CI_USE_FASTRTPS_DYNAMIC? by default, CI does not run this one.

[fujitatomoya]

@suurjaak

could you check test failures https://ci.ros2.org/job/ci_windows/17188/testReport/? it seems to be unrelated, but just in case.

https://ci.ros2.org/job/ci_windows/17188/cmake/folder.114148/ and https://ci.ros2.org/job/ci_windows/17188/msbuild/new/ are unrelated for sure.

[suurjaak]

@fujitatomoya anything specific from those 79 failures? I looked over a few dozen items and they all seemed unrelated.

[fujitatomoya]

I checked everything, but none of them seems to be related.

I want to be sure on this before merge, see if foxy master CI has the same unstable problem.

CI (foxy original):

• Windows

[fujitatomoya]

Okay now mainline(foxy branch) has the same unstable result. I will go ahead to merge this into foxy.