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

Fix disconnector use-after-free access after signal move construction #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

W4RH4WK
Copy link

@W4RH4WK W4RH4WK commented Sep 16, 2022

The disconnector object is no longer stored directly in the signal object. It is now stored on the heap and managed by the shared pointer that was already present to enable weak pointers, but did not actually manage the disconnector object.

fixes #24

The disconnector object is no longer stored directly in the signal object.
It is now stored on the heap and managed by the shared pointer that was already present to enable weak pointers, but did not actually manage the disconnector object.

fixes fr00b0#24
Copy link
Contributor

@kevin-- kevin-- left a comment

Choose a reason for hiding this comment

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

makes sense to me

@@ -17,10 +17,8 @@ namespace nod {
/// Interface for type erasure when disconnecting slots
struct disconnector {
virtual void operator()( std::size_t index ) const = 0;
virtual ~disconnector() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

pedantic, could use = default

Suggested change
virtual ~disconnector() {}
virtual ~disconnector() = default;

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. I am willing to change that when/if I get feedback form the maintainer.

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.

Signals created via move constructor/assignment use the moved from object
2 participants