ETLCPP / etl

Embedded Template Library
https://www.etlcpp.com
MIT License
2.25k stars 393 forks source link

etl/delegate: fix accident creation of a delegate to an rvalue delegate when copying/assigning from delegate with mismatching signature #965

Closed VladimirP1 closed 1 month ago

VladimirP1 commented 1 month ago

Original problem, how to reproduce:

#include <etl/delegate.h>

namespace ctx {
struct os {};
struct any {
  any() = default;
  any(os) {}
};
} // namespace ctx

struct Test {
    etl::delegate<void(ctx::os)> GetCb() {
      return etl::delegate<void(ctx::any)>::create<Test,
      &Test::CbAnyCtx>(
          *this);
    }

private:
  void CbAnyCtx(ctx::any ctx) {}
};

int main() {
  Test().GetCb()(ctx::os{});
  etl::delegate<void(void)> f = []() {};
  return 0;
}

This compiles and does not look suspicious at the first glance but results in a segmentation fault at runtime.

What happens here: 1) A delegate is created etl::delegate<void(ctx::any ctx)>::create<Test, &Test::CbAnyCtx>(*this); 2) But we need a etl::delegate<void(ctx::os ctx)>, the compiler selects the constructor from const TLambda&. 3) Note that etl::is_same<etl::delegate<TReturn(TParams...)>, TLambda>::value is false because the signatures of the delegates do not match 4) The newly created delegate is returned and the first delegate is destroyed. Now the second delegate holds a dangling pointer

This PR attempts to fix this issue by using a reliable method for checking if a functor is a delegate

jwellbelove commented 1 month ago

Thanks, I'll take a look at that when I get back from Spain later this week.

VladimirP1 commented 1 month ago

No other use, I simply did not want to pollute the namespace with additional classes

VladimirP1 commented 1 month ago

Should I replace delegate<void> with a tag class?

jwellbelove commented 1 month ago

Yes, I think so.

jwellbelove commented 1 month ago

It may be good to make a traits class.

template etl::is_delegate;

jwellbelove commented 1 month ago

Just thought of another addition.

etl::is_delegate_v definition. For C++17 only.

jwellbelove commented 1 month ago

I should be able to process this pull request tomorrow afternoon.