UB when passing a temporary lambda to a delegate

[denravonska]

We have a DMA driver that allows you to pass a lambda callback to call when the transfer has completed. When compiling for host with ASAN enabled we noticed that when passing a temporary lambda that captures:

    OS::BinarySemaphore dmaDone;
    dma.StartM2M(
        Driver::DMA::DmaCallback([&dmaDone]() {
            dmaDone.GiveFromISR();
        })
    );

ASAN will complain about stack use after scope end. If we extract the lambda to a variable it works just fine:

    OS::BinarySemaphore dmaDone;
    auto cb = Driver::DMA::DmaCallback([&dmaDone]() {
        dmaDone.GiveFromISR();
    });

    dma.StartM2M(cb);

It makes sense since delegate only references the callable passed to it, and when the callable goes out of scope it has to yolo it. I did some quick testing and deleting the constructor that accepts a moved lambda prevents you from making this mistake.

    template <typename TLambda, typename = etl::enable_if_t<etl::is_class<TLambda>::value && !etl::is_same<etl::delegate<TReturn(TParams...)>, TLambda>::value, void>>
    ETL_CONSTEXPR14 delegate(TLambda&& instance) = delete;

error.cpp:15:27: error: call to deleted constructor of 'etl::delegate<void ()>'
   15 |     etl::delegate<void()> callable([&someValue](){
      |                           ^        ~~~~~~~~~~~~~~~
   16 |         someValue = true;
      |         ~~~~~~~~~~~~~~~~~
   17 |     });
      |     ~
repo/include/etl/private/delegate_cpp11.h:121:21: note: 'delegate<(lambda at error.cpp:15:36), void>' has been explicitly marked deleted here
  121 |     ETL_CONSTEXPR14 delegate(TLambda&& instance) = delete;

I don't know if it's the right or wrong solution to this but it does make sense that you don't want to reference a moved lambda if you don't want to take ownership of it.

[jwellbelove]

Deleting the rvalue reference constructor seems to do the trick and doesn't cause any problems with the unit tests, except for the ones that this change is designed to fix. So even some of the unit tests were relying on UB!

[denravonska]

Deleting the rvalue reference constructor seems to do the trick and doesn't cause any problems with the unit tests, except for the ones that this change is designed to fix. So even some of the unit tests were relying on UB!

Out of curiosity, are you building the tests with ASAN enabled?

[jwellbelove]

I sometimes run the Linux unit test builds with -fsanitize=address,undefined,bounds and -O2, but I don't get any errors flagged. I do the same with VS2022 with the sanitizer turned on and -O2, and do not get errors either.

[jwellbelove]

I don't run the sanitizer option all of the time as the tests can take a very long time to run.

[denravonska]

Wait, maybe I'm missing something, the test suite seems super fast.

[marco@arch build]$ time ./etl_tests 
/home/marco/dev/app-sdk/third-party/etl/repo/test/../include/etl/span.h:428:32: runtime error: reference binding to null pointer of type 'struct span'
/home/marco/dev/app-sdk/third-party/etl/repo/test/../include/etl/span.h:429:30: runtime error: reference binding to null pointer of type 'struct span'
/home/marco/dev/app-sdk/third-party/etl/repo/test/../include/etl/multi_span.h:121:17: runtime error: reference binding to null pointer of type 'struct Data'
Success: 9582 tests passed.
Test time: 5.06 seconds.

real	0m5.503s
user	0m5.125s
sys	0m0.369s

Granted, it's not as fast as without sanitizer enabled, but still impressively fast given the number of tests.

[marco@arch build]$ ./etl_tests 
Success: 9582 tests passed.
Test time: 2.57 seconds.

Unsure why ASAN didn't complain about the lambdas in this case.

[jwellbelove]

I run the sanitizer with optimisations enabled. Plus the script runs the tests with various setups, for GCC and Clang. I'm also running the Linux tests under Windows WSL2.