boostorg / phoenix

Boost.org phoenix module
http://boost.org/libs/phoenix
28 stars 46 forks source link

Fix UB due to dangling references in try/catch #94

Closed mgaunard closed 4 years ago

djowel commented 4 years ago

LGTM. Waiting for CI to cycle (asuming it will :-)

mgaunard commented 4 years ago

Can we get this in 1.73 please?

Kojoley commented 4 years ago

@mgaunard Your changes look reasonable, but could you maybe add some tests that show asan fails before the PR? I have added asan to the CI and it currently did not find issues, and I have not come with a reproducer myself.

mgaunard commented 4 years ago

I do not think I will have the time to do a proper one before tomorrow the deadline for boost 1.73.

ASan is a tool that can help in finding bugs, it certainly cannot act as a control to check whether bugs are present or not, so I'm not sure what value you're expecting to get there. The bug itself is quite obvious from a glance at the code, and to be honest I'm quite concerned this went past the review process.

try_catch itself has no tests as-is. I suppose an easy example would be something like this (not tested)

#include <boost/phoenix.hpp>
#include <cassert>

auto make_func()
{
    namespace phoenix = boost::phoenix;
    using namespace phoenix::placeholders;
    return phoenix::try_[phoenix::throw_(arg1)].catch_all[phoenix::ref(arg2) = true];
}

int main()
{
    auto const f = make_func();

    bool thrown = false;
    f(42, thrown);
    assert(thrown == true);
}

Clearly the "throw arg1" and "arg2 = true" sub-expressions don't exist anymore when f is called. I made it into a separate function returning another function to increase the chances of ASan detecting it.

djowel commented 4 years ago

I'll have this merged. Let's deal with the test later.