👩‍🌾 what(): Character value exceeds maximum value benchmark_string_conversions regression

[claraberendsen]

Bug report

There is a regression that seems to have been introduced by the change implemented #114 that is affecting Rolling and Jazzy benchmark jobs.
This does not seem to be related to the OS change of the J and R jobs to noble, since the first failurereference occurred on a jammy job on Mar 27th a day after of the merge of the PR. It has been consistent since that date.
The error message is:

terminate called after throwing an instance of 'eprosima::fastcdr::exception::BadParamException'
  what():  Character value exceeds maximum value

Test failing
projectroot.benchmark_string_conversions
projectroot.benchmark_string_conversions

Reference builds

• https://build.ros2.org/job/Rci__benchmark_ubuntu_noble_amd64/54
• https://build.ros2.org/job/Jci__benchmark_ubuntu_noble_amd64/19

Required Info:

• Operating System:
  • Noble

Steps to reproduce issue

Run a build of rolling or jammy benchmark jobs

Expected behavior

Tests to pass

Actual behavior

Test fail consistently

[sloretz]

I can reproduce. I don't have a solution yet, but here's what I've found so far.

colcon build --packages-up-to rosidl_typesupport_fastrtps_c
colcon build --packages-select rosidl_typesupport_fastrtps_c --cmake-args  -DAMENT_RUN_PERFORMANCE_TESTS=ON -DCMAKE_BUILD_TYPE=Debug
colcon test --event-handlers console_direct+ --packages-select rosidl_typesupport_fastrtps_c --ctest-args -R benchmark_string_conversions

Run under GDB the relevant part of backtrace:

#9  0x0000563886722396 in rosidl_typesupport_fastrtps_c::cdr_deserialize (cdr=..., u16str=...)
    at /workspaces/ros2/src/ros2/rosidl_typesupport_fastrtps/rosidl_typesupport_fastrtps_c/include/rosidl_typesupport_fastrtps_c/serialization_helpers.hpp:71
#10 0x00005638867219a9 in PerformanceTest_wstring_to_u16string_Benchmark::BenchmarkCase (
    this=0x56389b7ea4f0, st=...)
    at /workspaces/ros2/src/ros2/rosidl_typesupport_fastrtps/rosidl_typesupport_fastrtps_c/test/benchmark/benchmark_string_conversions.cpp:56

Frame 9 context

rosidl_typesupport_fastrtps/rosidl_typesupport_fastrtps_c/include/rosidl_typesupport_fastrtps_c/serialization_helpers.hpp

Lines 52 to 77 in 4fd7bc3

	inline bool cdr_deserialize(
	eprosima::fastcdr::Cdr & cdr,
	rosidl_runtime_c__U16String & u16str)
	{
	uint32_t len;
	cdr >> len;
	bool succeeded = rosidl_runtime_c__U16String__resize(&u16str, len);
	if (!succeeded) {
	return false;
	}
	for (uint32_t i = 0; i < len; ++i) {
	// We are serializing a uint32_t for interoperability with other DDS-based implementations.
	// If we change this to a uint16_t in the future, we could remove the check below, since all
	// serialized values would fit in the destination type.
	uint32_t c;
	cdr >> c;
	if (c > std::numeric_limits<uint_least16_t>::max()) {
	throw eprosima::fastcdr::exception::BadParamException(
	"Character value exceeds maximum value");
	}
	u16str.data[i] = static_cast<uint_least16_t>(c);
	}
	return true;
	}

(gdb) info locals
c = 2752554
i = 0
len = 1024
succeeded = true

That value of c is definitely too large

$ cat asdf.cpp 
#include <limits>
#include <iostream>
#include <cstdint>

int main() {
        std::cout << std::numeric_limits<uint_least16_t>::max() << "\n";
        return 0;
}

$ g++ asdf.cpp && ./a.out
65535

[sloretz]

The test string is the character * repeated infinitely. The ASCII value of * is 0x2A. In 16 bits of hex would be 0x002A. The value of c is 2752554 which in 32 bits of hex is 0x002A002A. It looks to me like the following lines must be reading 32 bits from cdr in 2.2.x, and must have only read 16 bits in FastCDR 1.1.x.

rosidl_typesupport_fastrtps/rosidl_typesupport_fastrtps_c/include/rosidl_typesupport_fastrtps_c/serialization_helpers.hpp

Lines 67 to 68 in 4fd7bc3

	uint32_t c;
	cdr >> c;

Not sure what the right behavior should be. I'm looking through the changes to see what caused it https://github.com/eProsima/Fast-CDR/compare/1.1.x..2.2.x to see if I can find more info about the change.

[sloretz]

@MiguelCompany mind taking a look? Here's a minimal reproducible example showing the behavior difference between fastcdr 1.1.x and 2.2.x. Which behavior is correct?

package.xml

<?xml version="1.0"?>
<?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
<package format="3">
  <name>cdr_wchar</name>
  <version>0.1.0</version>
  <description>
    Comparing behavior between FastCDR 1.1.x and 2.2.xsd
  </description>
  <maintainer email="[email protected]">Foo bar</maintainer>
  <license file="LICENSE">Apache 2.0</license>

  <buildtool_depend>cmake</buildtool_depend>
  <depend>fastcdr</depend>

  <export>
    <build_type>cmake</build_type>
  </export>
</package>

CMakeLists.txt

cmake_minimum_required(VERSION 3.15)

project(cdr_wchar)

find_package(fastcdr REQUIRED CONFIG)

add_executable(wchar-test wchar.cpp)
target_link_libraries(wchar-test fastcdr)

wchar.cpp

#include <cstdint>
#include <iostream>
#include <string>

#include "fastcdr/Cdr.h"

constexpr const uint64_t kSize = 10;
namespace fastcdr = eprosima::fastcdr;

int main() {
    std::wstring foo_wstr(kSize, '*');
    std::cout << "foo_wstr is: '";
    std::wcout << foo_wstr;
    std::cout << "'\n";

    char raw_buffer[kSize * 4 + 4]; // 4 bytes per character + 4 bytes for the length
    fastcdr::FastBuffer buffer(raw_buffer, sizeof(raw_buffer));

    std::cout << "Using FastCDR version " << FASTCDR_VERSION_MAJOR << "." << FASTCDR_VERSION_MINOR << "." << FASTCDR_VERSION_MICRO << "\n";
    #if FASTCDR_VERSION_MAJOR == 2
    fastcdr::Cdr cdr(buffer, fastcdr::Cdr::DEFAULT_ENDIAN, fastcdr::CdrVersion::XCDRv1);
    cdr.set_encoding_flag(fastcdr::EncodingAlgorithmFlag::PLAIN_CDR);
    #else
    fastcdr::Cdr cdr(buffer, fastcdr::Cdr::DEFAULT_ENDIAN, fastcdr::Cdr::DDS_CDR);
    #endif

    // Hmm, seems odd that the benchmark setup doesn't ingest the length,
    // but the benchmark loop expects the length to be provided.
    cdr << foo_wstr;

    // Performed in every benchmark loop
    cdr.reset();

    // from cdr_deserialize
    uint32_t len;
    cdr >> len;
    std::cout << "Got length " << len << "\n";

    for (uint32_t i = 0; i < len; ++i) {
        uint32_t c;
        cdr >> c;
        std::cout << c << "\n";
    }

    return 0;
}

Results in this for 1.1.x

foo_wstr is: '**********'
Using FastCDR version 1.1.1
Got length 10
42
42
42
42
42
42
42
42
42
42

And this for 2.2.x

foo_wstr is: '**********'
Using FastCDR version 2.2.4
Got length 10
2752554
2752554
2752554
2752554
2752554
3403154240
32556
1651076199
779647075
73728

[MiguelCompany]

Not sure what the right behavior should be. I'm looking through the changes to see what caused it https://github.com/eProsima/Fast-CDR/compare/1.1.x..2.2.x to see if I can find more info about the change.

From the release notes on v2.0.0:
wchar types of size 2 as specified in the XTypes v1.3 specification instead of size 4 (eProsima/Fast-CDR#156)

So yes, that behavior changed from 1.x to 2.x.

That said, I don't think it affects ROS 2 at all, since it is not using std::wstring directly:

• For C++, std::u16string is used, and (de)serialization is performed by these methods
• For C, rosidl_runtime_c__U16String is used, and (de)serialization is performed by these methods

The only concern would then be the interoperability of wstring IDL definitions with plain DDS applications.

One way out of this would be to change the (de)serializer methods to (de)serialize 16-bit chars instead of 32-bit ones.
Since both rmw_fastrtps and rmw_connextdds use the type support from this repo, they will keep interoperating.
This would probably require changes in rmw_cyclonedds so it uses the same size for the wide characters.

[sloretz]

That said, I don't think it affects ROS 2 at all, since it is not using std::wstring directly

What should we do for the benchmark test then? Replace use of std::wstring with a call to cdr_serialize() on an rosidl_runtime_c__U16String in the setup section?

rosidl_typesupport_fastrtps/rosidl_typesupport_fastrtps_c/test/benchmark/benchmark_string_conversions.cpp

Lines 38 to 43 in 4fd7bc3

	std::wstring wstring(kSize, '*');
	char raw_buffer[kSize * 4 + 4]; // 4 bytes per character + 4 bytes for the length
	fastcdr::FastBuffer buffer(raw_buffer, sizeof(raw_buffer));
	fastcdr::Cdr cdr(buffer, fastcdr::Cdr::DEFAULT_ENDIAN, fastcdr::CdrVersion::XCDRv1);
	cdr.set_encoding_flag(fastcdr::EncodingAlgorithmFlag::PLAIN_CDR);
	cdr << wstring;

[MiguelCompany]

What should we do for the benchmark test then?

The test was added in #52 to benchmark the conversion functions we deprecated in #113.
The test was adapted to avoid using the deprecated functions. Do you think it should be removed instead?
Should we also remove the tests added in #43?

[Blast545]

🧑‍🌾 @sloretz ping jic

[MiguelCompany]

@sloretz Friendly ping

[MiguelCompany]

@sloretz @claraberendsen @Blast545 I went ahead and opened #122 that removes the benchmark tests.

[Blast545]

@MiguelCompany Thank you!