Rapidyaml calls abort() on parse failures of declarative config

[BugelNiels]

Describe your environment
AlmaLinux release 9.7, opentelemetry-cpp version 1.24

Steps to reproduce
Initialise the OpenTelemetry SDK through the declarative config yaml:

  auto model = opentelemetry::sdk::configuration::YamlConfigurationParser::ParseFile(configFile);

Then make the config non-compliant yaml:

    file_format: "1.0-rc.1"
    disabled: false
    # indented too many times
      resource: 
      attributes:
        - name: service.namespace
          value: mynamespace
          type: string
      schema_url: https://opentelemetry.io/schemas/1.16.0

The result will be:

Aborted (core dumped)

The resulting core dump shows the abort() call from ryml:

(gdb) bt
#0  0x00007fbf0659a02c in __pthread_kill_implementation () from /lib64/libc.so.6
#1  0x00007fbf0654cb86 in raise () from /lib64/libc.so.6
#2  0x00007fbf06536873 in abort () from /lib64/libc.so.6
#3  0x00007fbf056f8a28 in c4::yml::error_impl(char const*, unsigned long, c4::yml::Location, void*) () from /lib/libryml.so.0.9.0
#4  0x00007fbf05711f37 in void c4::yml::ParseEngine<c4::yml::EventHandlerTree>::_err<>(c4::basic_substring<char const>) const () from /lib/libryml.so.0.9.0
#5  0x00007fbf0571382b in c4::yml::ParseEngine<c4::yml::EventHandlerTree>::_scan_scalar_plain_blck(c4::yml::ParseEngine<c4::yml::EventHandlerTree>::ScannedScalar*, unsigned long) () from /lib/libryml.so.0.9.0
#6  0x00007fbf05722896 in c4::yml::ParseEngine<c4::yml::EventHandlerTree>::_handle_map_block() () from /lib/libryml.so.0.9.0
#7  0x00007fbf0572c9b5 in c4::yml::ParseEngine<c4::yml::EventHandlerTree>::parse_in_place_ev(c4::basic_substring<char const>, c4::basic_substring<char>) () from /lib/libryml.so.0.9.0
#8  0x00007fbf0570522a in c4::yml::parse_in_arena(c4::yml::ParseEngine<c4::yml::EventHandlerTree>*, c4::basic_substring<char const>, c4::basic_substring<char const>) () from /lib/libryml.so.0.9.0
#9  0x00007fbf090f0670 in opentelemetry::v1::sdk::configuration::RymlDocument::ParseDocument(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
   from /lib64/libopentelemetry_configuration.so
#10 0x00007fbf090f08c4 in opentelemetry::v1::sdk::configuration::RymlDocument::Parse(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
   from /lib64/libopentelemetry_configuration.so
#11 0x00007fbf090ee909 in opentelemetry::v1::sdk::configuration::YamlConfigurationParser::ParseString(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
   from /lib64/libopentelemetry_configuration.so
#12 0x00007fbf090eeebc in opentelemetry::v1::sdk::configuration::YamlConfigurationParser::ParseFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) () from /lib64/libopentelemetry_configuration.so

What is the expected behavior?
Parsing should terminate somewhat gracefully by e.g. throwing an exception that can be caught.

What is the actual behavior?
The ryml internals seem to be calling abort(), causing the process to terminate and core dump if parsing fails.

Additional context
During my build of opentelemetry-cpp, it will fetch ryml from the repo:

ryml: 0.9.0 (fetch_repository)

As such, a possible fix could be updating https://github.com/open-telemetry/opentelemetry-cpp/blob/main/cmake/ryml.cmake#L23 to add:

set(RYML_DEFAULT_CALLBACK_USES_EXCEPTIONS ON CACHE BOOL "" FORCE)

If this is an acceptable solution, I can create a PR.

_{Tip: React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it. Learn more here.}

[marcalff]

As such, a possible fix could be updating https://github.com/open-telemetry/opentelemetry-cpp/blob/main/cmake/ryml.cmake#L23 to add:

set(RYML_DEFAULT_CALLBACK_USES_EXCEPTIONS ON CACHE BOOL "" FORCE)

Thanks for the report and analysis.

Yes, this looks like the correct fix, PR welcome.

[BugelNiels]

Seems like we can only configure this behaviour with CMake; if the library is already installed or the building is done through Bazel, it will stay with the abort behaviour. This fix works for my use case, but I'm not sure if we want a more general "fix" here to ensure consistent behaviour.

Right now, it's also difficult to write any tests for this, as the behaviour changes depending on how ryml was pulled in.

[mehrdadbn9]

I can investigate and fix Rapidyaml abort on declarative config parse failure.

Plan:

• Locate Rapidyaml parsing code
• Find abort call issue
• Implement a fix

If maintainer thinks this needs fixing, please assign to me.

[mehrdadbn9]

I can fix the OpenTelemetry C++ rapidyaml crash issue. Plan: Update the YAML parsing logic to properly handle malformed input and prevent crashes. If maintainer thinks this needs fixing, please assign to me.

[marcalff]

Seems like we can only configure this behaviour with CMake; if the library is already installed or the building is done through Bazel, it will stay with the abort behaviour. This fix works for my use case, but I'm not sure if we want a more general "fix" here to ensure consistent behaviour.

Right now, it's also difficult to write any tests for this, as the behaviour changes depending on how ryml was pulled in.

@BugelNiels

Your fix is a good 1 line work around and unblocks the case where people build locally with cmake (which I do), which is why I merged it as is.

Looking at this in more depth, I agree with you that:

• the bazel build still needs to be fixed
• the fix should not rely on how the rapidyaml library was built, as opentelemetry-cpp does not control this.

This is now reported in #3857.

Please keep reporting issues as you find them, and/or contribute fixes if so inclined, both are appreciated.

@mehrdadbn9

Issue #3857 is open to anyone who wants to contribute, feel free to raise a PR.