Calling coro::sync_wait with coro::ring_buffer::consume returns default constructed objects for complex return values

[ripose-jp]

When calling coro::sync_wait() with the result of coro::ring_buffer::consume() it is possible for coro::sync_wait() to return a default constructed object instead of the actual object. This only seems to happen with complex objects as simple structs and basic types don't exhibit this behavior.

From what I can tell, this happens somewhere in tl_expected.hpp after returning from ring_buffer::consume_operation::await_return().

I've only ever been able to reproduce this bug using a protobuf message that contains a oneof field, so if this bug report seems highly specific, I apologize.

---

Here's a minimal example along with some build scripts to simplify the process of testing:

main.cpp

#include <coro/coro.hpp>

#include <iostream>

#include "example.pb.h"

int main(void)
{
    coro::ring_buffer<Example, 1> buffer;
    const auto foo = [&buffer] () -> coro::task<void>
    {
        Example data;
        Message *msg = data.mutable_message();
        msg->set_id(1);
        msg->set_text("Hello world!");
        std::cout << "Inside the coroutine\n";
        std::cout << "Message Case: " << static_cast<int>(data.Messages_case()) << '\n';
        std::cout << "ID: " << data.message().id() << '\n';
        std::cout << "Message: " << data.message().text() << '\n';
        co_await buffer.produce(std::move(data));
    };
    coro::sync_wait(foo());
    auto result = coro::sync_wait(buffer.consume());
    if (!result)
    {
        std::cout << "Consume failed!\n";
        return 0;
    }
    auto data = std::move(*result);
    std::cout << "Outside the coroutine\n";
    std::cout << "Message Case: " << static_cast<int>(data.Messages_case()) << '\n';
    std::cout << "ID: " << data.message().id() << '\n';
    std::cout << "Message: " << data.message().text() << '\n';
    return 0;
}

example.proto

syntax = "proto3";

message Message
{
    uint32 id = 1;
    string text = 2;
};

message Example
{
    oneof Messages
    {
        Message message = 1;
    }
}

CMakeLists.txt

cmake_minimum_required(VERSION 3.25)

project(
    libcoro-sync-wait-default-construct
    VERSION 1.0.0
    LANGUAGES CXX
)

find_package(Protobuf REQUIRED)

include(FetchContent)
set(LIBCORO_BUILD_EXAMPLES OFF)
set(LIBCORO_BUILD_TESTS OFF)
set(LIBCORO_FEATURE_TLS OFF)
set(LIBCORO_FEATURE_NETWORKING OFF)
FetchContent_Declare(
    libcoro
    GIT_REPOSITORY https://github.com/jbaldwin/libcoro.git
    GIT_TAG        0b65aa2b3a2b1227b6c74b59882f1afc95c91dcf
)
FetchContent_MakeAvailable(libcoro)

protobuf_generate_cpp(PROTO_SRCS PROTO_HDRS example.proto)
add_executable(main main.cpp ${PROTO_SRCS} ${PROTO_HDRS})
target_compile_features(main PRIVATE cxx_std_20)
target_compile_options(main PRIVATE -Wall -Wextra -Werror)
target_include_directories(main PRIVATE ${CMAKE_CURRENT_BINARY_DIR})
target_link_libraries(main PRIVATE libcoro protobuf::libprotobuf)

[jbaldwin]

I've got a PR open to upgrade tl::expected to the latest version as a start, it looks like it has some bug fixes, but nothing specifically calls out the behavior you are seeing.

https://github.com/TartanLlama/expected/releases

[jbaldwin]

TEST_CASE("ring_buffer issue-242 default constructed complex objects on consume", "[ring_buffer]")
{
    struct message
    {
        uint32_t id;
        std::string text;
    };

    struct example
    {
        std::optional<message> msg;
    };

    coro::ring_buffer<example, 1> buffer;

    const auto foo = [&buffer]() -> coro::task<void>
    {
        example data{};
        data.msg = {message{ .id = 1, .text = "Hello World!" }};
        std::cout << "Inside the coroutine\n";
        std::cout << "ID: " << data.msg.value().id << "\n";
        std::cout << "Text: " << data.msg.value().text << "\n";
        co_await buffer.produce(std::move(data));
        co_return;
    };

    coro::sync_wait(foo());
    auto result = coro::sync_wait(buffer.consume());
    REQUIRE(result);

    auto data = std::move(*result);
    REQUIRE(data.msg.has_value());
    REQUIRE(data.msg.value().id == 1);
    REQUIRE(data.msg.value().text == "Hello World!");
    std::cout << "Outside the coroutine\n";
    std::cout << "ID: " << data.msg.value().id << "\n";
    std::cout << "Text: " << data.msg.value().text << "\n";
}

This also seems to be reproducing the issue without protobuf, it fails on REQUIRE(data.msg.has_value())

[jbaldwin]

In sync_wait.hpp the sync_wait_task_promise::return_value function:

    auto return_value(return_type&& value) noexcept
    {
        m_return_value = std::addressof(value);
        return final_suspend();
    }

It takes a pointer to the return object instead of moving/taking ownership. Later when result() is called it then does std::move(*m_return_value). This seems to be the problem and causes the value to be default constructed instead of moving the original item out. I'll push a PR and I'd like you to test that it fixes your use case too if you don't mind.

[ripose-jp]

No problem. Just tag me when you push it. Thanks for figuring this out.

[jbaldwin]

Its going to take a little more time, the fix isn't as easy as I had anticipated, I know the root cause but fixing it is having some cascading issues that were already fixed on coro::task. I'm hoping to possibly port those fixes as well, or just possibly leverage coro::task for coro::sync_wait moving forward.

[ripose-jp]

No problem. There's no pressure to fix this. I just found a bug and though it was worth reporting. It's not currently affecting anything I'm working on.

[jbaldwin]

@ripose-jp I've added a PR that I think resolves the issue, the bug does appear to be in tl::expected and its usage around coro::sync_wait

https://github.com/jbaldwin/libcoro/pull/244/files#diff-3048842fe7373a54d457291bfc4c40ba9e84492042a6a087b17263d32bf3fce1R191

This is the important part of the PR if you want to take a look.

[jbaldwin]

After thinking about the current solution over night I'm not sure I want to merge this, but it does seem relatively clear its a bug in tl::expected? There is an identical usage of that line (that doesn't work) in coro::task for how it handles its return value and it doesn't have this problem.

[ripose-jp]

I'm personally not worried about your PR since it works with my example. This behavior is only only useful in unit testing for me since any production code dealing with coro::ring_buffer is going to be a coroutine. Having to do an extra move is unfortunate, but it's worth it if it produces correct behavior in my opinion.

If you're worried about it, replacing consume operations with coro::task<std::optional<T>> or something similar would be completely reasonable as well.

Fixing tl::expected works too, but since it seems abandoned by the developer, it would probably fall to you to keep it working. This might be more trouble than just avoiding the edge cases where it breaks.

[jbaldwin]

Hmm, maybe its time to just roll a coro::expected and use something like std::variant<T, E> internally with a simple interface to give it the same api as c++23's std::expected? Would drop the dependency and also as you noted tl::expected doesn't look particularly well maintained anymore. I dislike using std::optional<T> directly since it doesn't tell the user why the consume operation failed, even though there is really only 1 reason for why it failed at the moment. But as a user you'd need to read the docs to understand std::nullopt means you shut it down (which hopefully you know you did that...).

There is the option of upgrading to c++23 for std::expected but I'm not sure I want to do that yet, I figured this project should be at most 1 major version behind the latest C++ standard, and probably support two LTS releases on the OS distros. I should probably document that somewhere as well.

[ripose-jp]

coro::expected would be fine for my use case since I'm not in the position to upgrade to a C++23 compatible compiler for what I'm working on. Of course, its entirely your choice to move to C++23 when you choose, but if it happened soon, I would probably have to maintain my own C++20 compatible version for what I'm working on.

That said, I don't know what the difficulty of maintaining a std::expected compatible object would be, so maybe just using coro::task more generally around the code base would be easier than replacing tl::expected.

[jbaldwin]

Yeah I tried replacing the sync_task with just coro::task and it's more difficult than it seems. coro::task might need a way to support the initial suspend and final suspend awaiters generically to make it work properly, it's the one piece that is different from sync_task. On a side note Tlthis also gave me the thought to do something similar in coro::task_container in the cleanup task by using a final awaiter.

Fwiw I'm not planning on upgrading to c++23 anytime soon, would be 3+ years minimum for the next major standard drop, so it isn't a realistic solution today, just was mentioning it.

I think I'll take a stab at coro::expected and see how difficult it is for the singular ring buffer usage.

I appreciate the feedback and thoughts.

[ripose-jp]

I feel silly for not thinking of this earlier, but I just tested my example with C++23's std::expected and it exhibits the same behavior as tl::expected on GCC 13.1.0. It might be safe to rule tl::expected out as the culprit here unless both happen to be broken somehow.

[jbaldwin]

Hmm that is interesting. Good idea, I'll play around with it as well.

That does point to some kind of bug in libcoro then even though up until now it seems like it was tl::expected. Also confused why the same code in coro::task seems fine.

[jbaldwin]

So I tried it with std::optional<Element> and it fails similarly. It might be an issue with how coro::ring_buffer<e, n> is handling the objects?

[jbaldwin]

Ok I finally got smart and ran the test through valgrind, and it is a use after free bug.

1. Call sync_wait(awaitable)
2. coroutine executes and finishes
3. execute the return std::move(task).promise().result() line this is in theory moving the promise's return value but is probably just a temporary r-value, nothing "concrete".
4. sync_wait() returns and destructs all locals, this includes the ~task and the ~promise
5. Since the promise is destructed it also deletes the return value, we can see the destructor get called here
6. We then see a move operator= call on the return value type but it reads from the already free'ed memory location on the promise, this gives you the "empty" object in your example.

This probably works on simple types that fit in a register since its not calling the destuctor on the promise?

The solution with the double move gets the return value off the promise so it isn't destructed on the sync_wait() return (step 4) and allows the compiler to see that its being returned.

[ripose-jp]

I ran it with asan and it agrees with Valgrind about the use-after-free. I'd say you should merge your double move solution since correctness is going to be more important than performance. The only other way I can think of to get out of this is allocating return values in the heap since this bug doesn't affect trivially copyable types, but that comes with its own set of drawbacks.

[jbaldwin]

Agreed, I'm going to adjust some of the comments since it isn't a tl:: expected bug and then I'll get it merged.