Getting use-after-free sanitizer error with co-routines

[Kfir Gollan]

Hey,

I recently experimented with working with the http library and coroutines.

I have written a small program (based on http_client_demo.cc) which uses coroutines.

The example is rather straightforward, yet I get sanitizer error:

==1==ERROR: AddressSanitizer: heap-use-after-free on address 0x614000000900 at pc 0x7f129a84c1f0 bp 0x7ffef7dac310 sp 0x7ffef7dac300

Note that I also get this warning when the program starts

==1==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!

The code was compiled with latest seastar on debug mode.

I noticed that in previous threads it was noted that gcc doesn't support coroutines yet. Most comments were rather old, so I'm not sure whether this was resolved yet or not.
This is the gcc version I was using

$ gcc -v

Using built-in specs.

COLLECT_GCC=gcc

COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/12/lto-wrapper

OFFLOAD_TARGET_NAMES=nvptx-none:amdgcn-amdhsa

OFFLOAD_TARGET_DEFAULT=1

Target: x86_64-linux-gnu

Configured with: ../src/configure -v --with-pkgversion='Ubuntu 12.2.0-3ubuntu1' --with-bugurl=file:///usr/share/doc/gcc-12/README.Bugs --enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++,m2 --prefix=/usr -

-with-gcc-major-version-only --program-suffix=-12 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/

usr/lib --enable-nls --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-plugin --enable-defa

ult-pie --with-system-zlib --enable-libphobos-checking=release --with-target-system-zlib=auto --enable-objc-gc=auto --enable-multiarch --disable-werror --enable-cet --with-arch-32=i686 --with-abi=m64 --with-m

ultilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none=/build/gcc-12-U8K4Qv/gcc-12-12.2.0/debian/tmp-nvptx/usr,amdgcn-amdhsa=/build/gcc-12-U8K4Qv/gcc-12-12.2.0/deb

ian/tmp-gcn/usr --enable-offload-defaulted --without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu

Thread model: posix

Supported LTO compression algorithms: zlib zstd

gcc version 12.2.0 (Ubuntu 12.2.0-3ubuntu1)

Here is the relevant code base, the use-after-free is on the `co_await(in.consume...)` line.
Am I missing something in my implementation here, i.e is this a real bug on my end?

#include <seastar/core/app-template.hh>

#include <seastar/core/iostream.hh>

#include <seastar/core/future.hh>

#include <seastar/coroutine/all.hh>

#include <seastar/http/client.hh>

#include <seastar/http/request.hh>

#include <seastar/http/reply.hh>

#include <fmt/printf.h>

using namespace seastar;

struct printer {

    future<consumption_result<char>> operator() (temporary_buffer<char> buf) {

        if (buf.empty()) {

            return make_ready_future<consumption_result<char>>(stop_consuming(std::move(buf)));

        }

        fmt::print("{}", sstring(buf.get(), buf.size()));

        return make_ready_future<consumption_result<char>>(continue_consuming());

    }

};

future<int> http_tester() {

    auto req = http::request::make("GET", "127.0.0.1", "/");

    socket_address addr{ipv4_addr("127.0.0.1", 9090)};

    http::experimental::client http_client{addr};

    co_await http_client.make_request(std::move(req),

                                       [](const http::reply& rep, input_stream<char>&& in) mutable -> future<> {

                                           co_await in.consume(printer{});

                                           co_await in.close();

                                       });

    co_await http_client.close();

    co_return 0;

}

int main(int argc, char** argv) {

    app_template app;

    return app.run(argc, argv, [&]() -> future<int> { return http_tester(); });

}

[Kfir Gollan]

I ran the same test with clang++-15

clang++-15 --version

Ubuntu clang version 15.0.7

Target: x86_64-pc-linux-gnu

Thread model: posix

InstalledDir: /usr/bin

This time I didn't get the sanitizer warning printed to stdout/stderr, but it still failed on the same location.

I attached the full output of the program (with the sanitizer error) as a text file.

[Noah Watkins]

co_await http_client.make_request(std::move(req),
[](const http::reply& rep, input_stream<char>&& in) mutable -> future<> {
co_await in.consume(printer{});
co_await in.close();
});

In the context of make_request that invokes your lambda helper, `in` is a temporary input_stream (https://github.com/scylladb/seastar/blob/master/src/http/client.cc#L317). So its lifetime won’t survive past the first co_await. You could probably move `rep` and `in` onto the stack in your lambda, or avoid using coroutine lambdas, or I’m vaguely aware there are some helpers in the seastar tree for working with lambda coroutines.

> --
> You received this message because you are subscribed to the Google Groups "seastar-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/bcb88292-ceba-48d8-a63f-a5c57b58df20n%40googlegroups.com.
> <data.txt>

[Kfir Gollan]

Thanks for pointing that out, I have verified that creating a local copy of the `input_stream<char>&& in` prevents the use-after-free issue.
However, as I understand, the fact that `in` is a temporary variable in the `make_request` is because of the reference counting.
By creating a local copy of the input_stream, which is a part of the connection, I am in fact breaking the reference counting mechanism no?

Adding Pavel (xemul) to the loop as he worked on this area recently.

Analysis:

The `with_connection` uses `get_connection` to get a connection pointer (shared pointer). It then invokes the callback (fn) with the value (i.e the actual connection).

The instance is then returned to the pool in the `put_connection` flow.

template <typename Fn>

SEASTAR_CONCEPT( requires std::invocable<Fn, connection&> )

auto client::with_connection(Fn&& fn) {

    return get_connection().then([this, fn = std::move(fn)] (connection_ptr con) mutable {

        return fn(*con).then_wrapped([this, con = std::move(con)] (auto f) mutable {

            return put_connection(std::move(con), !f.failed()).then([f = std::move(f)] () mutable {

                return std::move(f);

            });

        });

    });

}

The implementation of get_connection returns a shared pointer to the given connection

future<client::connection_ptr> client::get_connection() {

    if (!_pool.empty()) {

        connection_ptr con = _pool.front().shared_from_this();

        _pool.pop_front();

        http_log.trace("pop http connection {} from pool", con->_fd.local_address());

        return make_ready_future<connection_ptr>(con);

    }

    if (_nr_connections >= _max_connections) {

        return _wait_con.wait().then([this] {

            return get_connection();

        });

    }

    return _new_connections->make().then([cr = internal::client_ref(this)] (connected_socket cs) mutable {

        http_log.trace("created new http connection {}", cs.local_address());

        auto con = seastar::make_shared<connection>(std::move(cs), std::move(cr));

        return make_ready_future<connection_ptr>(std::move(con));

    });

}

[Kfir Gollan]

Did some more digging into the documentation to find whether there are some docs on the mentioned wrapper for lambda coroutines.
As it turns out there is such a page, the lambda-coroutine-fiasco. 

I tried to apply the method mentioned there for my scenario with the two options available in the page. Sadly both failed.

#include <seastar/core/app-template.hh>

#include <seastar/core/iostream.hh>

#include <seastar/core/future.hh>

#include <seastar/coroutine/all.hh>

#include <seastar/coroutine/maybe_yield.hh>

#include <seastar/http/client.hh>

#include <seastar/http/request.hh>

#include <seastar/http/reply.hh>

#include <fmt/printf.h>

using namespace seastar;

struct printer {

    future<consumption_result<char>> operator() (temporary_buffer<char> buf) {

        if (buf.empty()) {

            return make_ready_future<consumption_result<char>>(stop_consuming(std::move(buf)));

        }

        fmt::print("{}", sstring(buf.get(), buf.size()));

        return make_ready_future<consumption_result<char>>(continue_consuming());

    }

};

future<int> http_tester_v1() {

    auto req = http::request::make("GET", "127.0.0.1", "/");

    socket_address addr{ipv4_addr("127.0.0.1", 9090)};

    http::experimental::client http_client{addr};

    co_await http_client.make_request(std::move(req),

                                       seastar::coroutine::lambda([](const http::reply& rep, input_stream<char>&& in) mutable -> future<> {

                                            co_await seastar::coroutine::maybe_yield();

                                            co_await in.consume(printer{});

                                            co_await in.close();

                                       }));

    co_await http_client.close();

    co_return 0;

}

future<int> http_tester_v2() {

    auto req = http::request::make("GET", "127.0.0.1", "/");

    socket_address addr{ipv4_addr("127.0.0.1", 9090)};

    http::experimental::client http_client{addr};

    auto a_lambda = [] (const http::reply& rep, input_stream<char>&& in) -> future<> {

        co_await seastar::coroutine::maybe_yield();

        co_await in.consume(printer{});

        co_await in.close();

    };

    co_await http_client.make_request(std::move(req), std::ref(a_lambda));

    co_await http_client.close();

    co_return 0;

}

int main(int argc, char** argv) {

    app_template app;

    return app.run(argc, argv, [&]() -> future<int> { return http_tester_v2(); });

}

The first approach failed with

==1==ERROR: AddressSanitizer: stack-use-after-return on address 0x7f0b5bc93510 at pc 0x55e17bfebf36 bp 0x7ffcbd1e5360 sp 0x7ffcbd1e5358

The second approach failed with

==1==ERROR: AddressSanitizer: stack-use-after-return on address 0x7fb9a1393110 at pc 0x557f5060af36 bp 0x7fff757ec4a0 sp 0x7fff757ec498

Seeing that both Avi and Tomasz were involved in the said file, adding both of them to the discussion.

The original discussion on the mailing list is available here.

Thanks again for all the help,

Kfir

[Gleb Natapov]

On Sun, Jul 30, 2023 at 11:57:42AM +0300, Kfir Gollan wrote:
> Did some more digging into the documentation to find whether there are some
> docs on the mentioned wrapper for lambda coroutines.
> As it turns out there is such a page, the lambda-coroutine-fiasco

> <https://github.com/scylladb/seastar/blob/0784da87690c79afabd8de8d20fbe633740bf0e0/doc/lambda-coroutine-fiasco.md>
> .

>
> I tried to apply the method mentioned there for my scenario with the two
> options available in the page. Sadly both failed.

The problem described there is not applicable to you case. In your case
first the co-routine lambda does not have any captures, so its lifetime is
irrelevant and second it is copied into noncopyable_function anyway
since this is what make_request() excepts.

In your code the problem seams to be access to the 'in' after the yield.
Since 'in' is rvalue it is stored as such in the co-routine frame and
this does not prevent it from been freed.

> <https://groups.google.com/g/seastar-dev/c/RFGF_a81lG4/m/_nSvG_E8AAAJ>.

>
> Thanks again for all the help,
> Kfir
>
>
> On Sat, Jul 29, 2023 at 8:14 PM Kfir Gollan <kfir....@gmail.com> wrote:
>
> > Thanks for pointing that out, I have verified that creating a local copy
> > of the `input_stream<char>&& in` prevents the use-after-free issue.
> > However, as I understand, the fact that `in` is a temporary variable in
> > the `make_request` is because of the reference counting.
> > By creating a local copy of the input_stream, which is a part of the
> > connection, I am in fact breaking the reference counting mechanism no?
> >
> > Adding Pavel (xemul) to the loop as he worked on this area recently.
> >
> > Analysis:
> >
> > The `with_connection` uses `get_connection` to get a connection pointer

> > (shared pointer). It then invokes the callback (fn) with *the value* (i.e

> To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/CA%2BONSZOhK%2B526Q2epwJtrtdsyMdLE4gJnVwGEAEZWhaf0e5S-Q%40mail.gmail.com.

--
Gleb.