returning r-value reference to stack allocated type (memory corruption)

[saurik]

I have been setting up to run clang-tidy against my codebase, and during my "this isn't even integrated into my build environment yet" one-off test of a totally unconfigured clang-tidy, it popped up with a single issue that seems to be a legitimate error in this codebase ;P.

BeastHttp/BeastHttp/include/http/reactor/ssl/impl/session.hxx

Line 671 in af4bd30

return std::move(ctx_);

session::handshake returns something that kind of feels like a typo--a decltype over a comma expression, though I'm guessing you are doing that as a different way to simulate std::enable_if?--resulting in what is effectively decltype(std::declval<context_type>()), which definitely isn't how std::declval is supposed to be used, and results in an r-value reference.

You then std::move a stack variable into that r-value reference, but the stack variable is going to be deallocated before anyone can consume that. Without having read any of the rest of the code, I feel like maybe you actually wanting to return a concrete context_type, to which you don't need to std::move (as you will get a named return value optimization on the result).

Regardless, here is the output from clang-tidy.

./BeastHttp/BeastHttp/include/http/reactor/ssl/impl/session.hxx:671:5: warning: Address of stack memory associated with local variable 'ctx_' returned to caller [clang-analyzer-core.StackAddressEscape]
    return std::move(ctx_);
    ^
server.cpp:746:9: note: Calling 'session::handshake'
        SslHttpSession::handshake(context, std::move(socket), router, [](auto context) {
        ^
./BeastHttp/BeastHttp/include/http/reactor/ssl/impl/session.hxx:684:12: note: Calling 'session::handshake'
    return handshake(ctx, std::move(socket), router, std::move(buffer),
           ^
./BeastHttp/BeastHttp/include/http/reactor/ssl/impl/session.hxx:671:5: note: Address of stack memory associated with local variable 'ctx_' returned to caller
    return std::move(ctx_);
    ^
Suppressed 1 warnings (1 in non-user code).

[0xdead4ead]

Sorry to be late with an answer!
Unfortunately, I could not find a suitable solution to saved SFINAE support and returned prvalue from static constructors. I would appreciate if you help me with this. 🙂

[saurik]

I am seeing two simple options for your use case here: the first one will require making a new template (as I'm not sure if there's an off-the-shelf definition of this one), but is conceptually direct; the second is "the minimal hack to the thing you are currently doing that will at least make it work".

1. Given the following definition, you should be able to change decltype(X, std::declval<Y>()) (which returns Y&&) to typename enable_for<X, Y>::type (which returns Y).

template <typename Check_, typename Type_>
struct enable_for {
    typedef Type_ type;
};

2. I think you also should just be able to wrap your existing decltype(X, std::declval<Y>()) in typename std::decay<...>::type, as that will take the Y&& and decay it to Y.

-> typename std::decay<decltype(
BEASTHTTP_REACTOR_SSL_SESSION_TRY_INVOKE_FLESH_TYPE(std::declval<Router const&>()),
std::declval<context_type>())>::type

[saurik]

Oh wait! I'm sorry... that first suggestion is wrong as I forgot an extra level of decltype: the correct code would be typename enable_for<decltype(X), Y>::type.

But also, in realizing the underlying mistake I made while suggesting that, it occurred to me that there's a more direct way to use the more built-in STL features.

typename stl::enable_if<(X, true), Y>::type

[0xdead4ead]

ok!
Replace the following macro definitions:

BEASTHTTP_REACTOR_SESSION_TRY_INVOKE_FLESH_TYPE(/*...*/)
BEASTHTTP_REACTOR_SESSION_TRY_INVOKE_FLESH_TYPE_LEGACY(/*...*/)

on this:

template<class Router, class = BEASTHTTP_REACTOR_SESSION_TRY_INVOKE_FLESH_TYPE(/*...*/)>
std::true_type try_invoke_flesh_type_helper(int);

template<class>
std::false_type try_invoke_flesh_type_helper(...);

and:

std::enable_if<decltype(try_invoke_flesh_type_helper<Router const&>(0))::value, context_type>::type

I like a second solution more, need to try it...

[saurik]

Thanks! I can report that the codebase (at least the parts I'm using) no longer experiences the clang-tidy warning I was reporting.