Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

VladimirP1
Copy link

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
Copy link
Contributor

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

Copy link
Contributor

@jwellbelove jwellbelove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does class delegate have any use apart from being a base class 'tag'?
It's mainly a style thing, but when I've done something like this before I've usually just created a simple 'tag' class.

struct delegate_tag {};

class delegate<void> sort of looks like it should do something useful as a delegate type.

@VladimirP1
Copy link
Author

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

@VladimirP1
Copy link
Author

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

@jwellbelove
Copy link
Contributor

Yes, I think so.

@jwellbelove
Copy link
Contributor

It may be good to make a traits class.

template
etl::is_delegate;

@jwellbelove
Copy link
Contributor

Just thought of another addition.

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

@jwellbelove
Copy link
Contributor

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

@jwellbelove jwellbelove changed the base branch from master to pull-request/#965-etl-delegate-fix-accident-creation-of-a-delegate October 18, 2024 15:48
@jwellbelove jwellbelove merged commit 71b691f into ETLCPP:pull-request/#965-etl-delegate-fix-accident-creation-of-a-delegate Oct 21, 2024
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants