Skip to content

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

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.

Sign up for GitHub

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

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

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 9 additions & 16 deletions include/nod/nod.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@kevin-- kevin-- Oct 4, 2023

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

@W4RH4WK W4RH4WK Oct 5, 2023

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.

};
/// Deleter that doesn't delete
inline void no_delete(disconnector*){
}
} // namespace detail

/// Base template for the signal class
Expand Down Expand Up @@ -364,10 +362,9 @@ namespace nod {
_slots = std::move(other._slots);
if(other._shared_disconnector != nullptr)
{
_disconnector = disconnector{ this };
// get ownership for disconnector and update back-reference
_shared_disconnector = std::move(other._shared_disconnector);
// replace the disconnector with our own disconnector
*static_cast<disconnector*>(_shared_disconnector.get()) = _disconnector;
static_cast<disconnector*>(_shared_disconnector.get())->_ptr = this;
}
}
/// signals are move assignable
Expand All @@ -381,10 +378,9 @@ namespace nod {
_slots = std::move(other._slots);
if(other._shared_disconnector != nullptr)
{
_disconnector = disconnector{ this };
// get ownership for disconnector and update back-reference
_shared_disconnector = std::move(other._shared_disconnector);
// replace the disconnector with our own disconnector
*static_cast<disconnector*>(_shared_disconnector.get()) = _disconnector;
static_cast<disconnector*>(_shared_disconnector.get())->_ptr = this;
}
return *this;
}
Expand Down Expand Up @@ -419,8 +415,7 @@ namespace nod {
_slots.push_back( std::forward<T>(slot) );
std::size_t index = _slots.size()-1;
if( _shared_disconnector == nullptr ) {
_disconnector = disconnector{ this };
_shared_disconnector = std::shared_ptr<detail::disconnector>{&_disconnector, detail::no_delete};
_shared_disconnector = std::make_shared<disconnector>(this);
}
++_slot_count;
return connection{ _shared_disconnector, index };
Expand Down Expand Up @@ -646,11 +641,9 @@ namespace nod {
std::vector<slot_type> _slots;
/// Number of connected slots
size_type _slot_count;
/// Disconnector operation, used for executing disconnection in a
/// type erased manner.
disconnector _disconnector;
/// Shared pointer to the disconnector. All connection objects has a
/// weak pointer to this pointer for performing disconnections.
/// Disconnector operation, used for executing disconnection. All
/// connection objects have a weak pointer to this shared pointer
/// for performing disconnections.
std::shared_ptr<detail::disconnector> _shared_disconnector;
};

Expand Down