serveFile crashes if passed-by-reference fileName parameter goes out of scope

[MatthewStephenson]

serveFile takes a const std::string& fileName parameter, which is a reference to a string passed in by the caller.

Async::Promise<ssize_t> serveFile(ResponseWriter& writer,
                                      const std::string& fileName,
                                      const Mime::MediaType& contentType)

It later uses that reference in the then() lambda attached to the transport->asyncWrite call (http.cc line 1133). This lambda is called in a different thread, by which time serveFile can have returned, and the caller's filename string can have gone out of scope or otherwise been deleted.

        return transport->asyncWrite(sockFd, buffer, MSG_MORE)  
            .then(
                [=](ssize_t) {
                    return transport->asyncWrite(sockFd, FileBuffer(fileName));  // <<<----------
                },
                Async::Throw);

This is causing a crash in my application. I have put together the following minimal application which exhibits the problem when compiled with address-sanitizer. After serving a couple of files, address-sanitizer detects the stack-use-after-scope.

g++ -std=c++2a -Wall -g -fsanitize=address -static-libasan -fno-omit-frame-pointer -o test test.cpp -lpistache

#include <pistache/common.h>
#include <pistache/endpoint.h>
#include <pistache/http.h>

using namespace Pistache;

class MyHandler : public Http::Handler
{   
    HTTP_PROTOTYPE(MyHandler)

    void onRequest(
        const Http::Request& req,
        Http::ResponseWriter response) override
    {
            const static std::string webroot = ".";

            if (req.method() == Http::Method::Get)
            {   
                std::string filename = webroot + req.resource();
                Http::serveFile(response, filename);
            }
    }
};

int main(int argc, char* argv[])
{   
    Port port(9080);

    Address addr(Ipv4::any(), port);

    Http::Endpoint server(addr);

    server.init(Http::Endpoint::options().threads(1).flags(Tcp::Options::ReuseAddr));
    server.setHandler(Http::make_handler<MyHandler>());
    server.serve();
}

==31231==ERROR: AddressSanitizer: stack-use-after-scope on address 0x6f8fd8c8 at pc 0x0013c490 bp 0x6f8fd77c sp 0x6f8fd774
READ of size 4 at 0x6f8fd8c8 thread T4
    #0 0x13c48f in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::size() const /usr/include/c++/8/bits/basic_string.h:931
    #1 0x155657 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_check_length(unsigned int, unsigned int, char const*) const /usr/include/c++/8/bits/basic_string.h:311  
    #2 0x156507 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_replace(unsigned int, unsigned int, char const*, unsigned int) /usr/include/c++/8/bits/basic_string.tcc:425
    #3 0x76edb593 in Pistache::RawBuffer::RawBuffer(char const*, unsigned int) (/usr/lib/arm-linux-gnueabihf/libpistache.so.0+0x9a593)
    #4 0x76edb5cf in Pistache::DynamicStreamBuf::buffer() const (/usr/lib/arm-linux-gnueabihf/libpistache.so.0+0x9a5cf)
    #5 0x76eae51b in Pistache::Http::ResponseWriter::putOnWire(char const*, unsigned int) (/usr/lib/arm-linux-gnueabihf/libpistache.so.0+0x6d51b)
    #6 0x76eaea1f in Pistache::Http::ResponseWriter::sendImpl(Pistache::Http::Code, char const*, unsigned int, Pistache::Http::Mime::MediaType const&) (/usr/lib/arm-linux-gnueabihf/libpistache.so.0+0x6da1f) 
    #7 0x76eaee9f in Pistache::Http::ResponseWriter::send(Pistache::Http::Code, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Pistache::Http::Mime::MediaType const&) (/usr/lib/arm-linux-gnueabihf/libpistache.so.0+0x6de9f)
    #8 0x76eaf85b in Pistache::Http::Handler::onInput(char const*, unsigned int, std::shared_ptr<Pistache::Tcp::Peer> const&) (/usr/lib/arm-linux-gnueabihf/libpistache.so.0+0x6e85b)
    #9 0x76ede16f in Pistache::Tcp::Transport::handleIncoming(std::shared_ptr<Pistache::Tcp::Peer> const&) (/usr/lib/arm-linux-gnueabihf/libpistache.so.0+0x9d16f)
    #10 0x76ee299f in Pistache::Tcp::Transport::onReady(Pistache::Aio::FdSet const&) (/usr/lib/arm-linux-gnueabihf/libpistache.so.0+0xa199f)
    #11 0x76eee12b in Pistache::Http::TransportImpl::onReady(Pistache::Aio::FdSet const&) (/usr/lib/arm-linux-gnueabihf/libpistache.so.0+0xad12b)
    #12 0x76eda4f3 in Pistache::Aio::SyncImpl::runOnce() (/usr/lib/arm-linux-gnueabihf/libpistache.so.0+0x994f3)
    #13 0x76ed7fef in std::thread::_State_impl<std::thread::_Invoker<std::tuple<Pistache::Aio::AsyncImpl::Worker::run()::{lambda()#1}> > >::_M_run() (/usr/lib/arm-linux-gnueabihf/libpistache.so.0+0x96fef)   
    #14 0x76d6d9af  (/usr/lib/arm-linux-gnueabihf/libstdc++.so.6+0x9d9af)

    #7 0x76e58fef in std::thread::_State_impl<std::thread::_Invoker<std::tuple<Pistache::Aio::AsyncImpl::Worker::run()::{lambda()#1}> > >::_M_run() (/usr/lib/arm-linux-gnueabihf/libpistache.so.0+0x96fef)
    #8 0x76d189af  (/usr/lib/arm-linux-gnueabihf/libstdc++.so.6+0x9d9af)

[dennisjenkins75]

Good find, excellent bug report.

Off the top of my mind (I've not looked at the classes involved), I suspect that one possible solution is to replace the filename string with a smart pointer of the same, then touch up all code that uses it.

[kiplingw]

Wouldn't the easiest thing to do for a fix is simply change the const std::string& fileName parameter (and possibly also the const Mime::MediaType& contentType as well) to pass by value? Neither are expensive to do this to, especially when using move semantics.

[dennisjenkins75]

Good point, and that is a simpler and more efficient change.

[kiplingw]

@MatthewStephenson, if you want to provide a PR, this should be an easy fix you can test. Also make sure you bump the major version in version.txt because this is changing an interface and won't be backwards compatible.

[MatthewStephenson]

@kiplingw The code change itself seems easy but I'm sufficiently unfamiliar with both the A+/async way of working and the github PR process that I'd rather leave it to someone more competent, if that's ok.

[tyler92]

It later uses that reference in the then() lambda attached to the transport->asyncWrite call

Hmm, but the string is captured by value (see [=]). I tried to reproduce with asan and tsan - the are no issues.
@MatthewStephenson is it still reproduced with the master branch?

[MatthewStephenson]

I could be wrong (I'm not at all hot on lambdas), but isn't it the reference to the string that is captured by value (effectively just a pointer) rather than the string itself?

[tyler92]

I could be wrong (I'm not at all hot on lambdas), but isn't it the reference to the string that is captured by value (effectively just a pointer) rather than the string itself?

No, it works this way only for pointers, not for references. So it's captured by value with this syntax. There is a short demo: https://godbolt.org/z/WeWPf3Pv9