Bug #2333
closedSignal: disconnecting a handler while it is executing is unsafe
100%
Description
It is not obvious that disconnecting the handler can deallocate it, thus leading to a use-after-free crash. It is even less obvious when the handler is a lambda, because wrapping it into a std::function
creates a copy, therefore the copy will be deallocated regardless of the lifetime of the original lambda closure. This effectively means that lambdas cannot be used as handlers if they need to disconnect themselves.
Possible solution:
In Signal<>::disconnect
, if m_isExecuting
is true, do not call m_slots.erase
. In Signal<>::operator()
delete line 198 and, after the handler invocation, add:
if (m_currentSlot == m_slots.end())
it = m_slots.erase(it);
else
++it;
This should keep the closure alive until after the handler has finished executing.
Updated by Davide Pesavento almost 10 years ago
- Related to Bug #2302: UtilSignal/DisconnectSelfInHandler segfault added
Updated by Davide Pesavento almost 10 years ago
- Related to Bug #2523: UtilSignal/DisconnectSelfInHandler use-after-free added
Updated by Davide Pesavento about 9 years ago
- Target version changed from v0.3 to v0.4
We're going to hit this issue in NFD when migrating from the old Face::onFail
to LpFace::afterStateChange
.
Updated by Junxiao Shi about 9 years ago
- Assignee set to Davide Pesavento
20151022 conference call approves Davide's solution.
Updated by Davide Pesavento about 9 years ago
- Status changed from New to In Progress
- % Done changed from 0 to 30
UtilSignal/DisconnectSelfInHandler fails the connection.isConnected() == false
check immediately after disconnecting. This is expected since the handler destruction is now deferred, therefore the weak_ptr
is "valid" till the end of the handler execution.
Updated by Junxiao Shi about 9 years ago
Reply to note-5:
This is bad when a handler .disconnect
itself twice: at the second invocation, it == m_currentSlot
assertion would fail.
It's wrong to add m_disconnect.reset()
at the end of Connection::disconnect
function, because other Connection
copies would still say it's connected.
How about: add it->disconnect.reset()
to Signal<..>::disconnect
in executing branch:
This immediately destroys the shared_ptr, so the weak_ptrs in Connection
s become invalid immediately.
Updated by Davide Pesavento about 9 years ago
- Status changed from In Progress to Code review
- % Done changed from 30 to 100
Updated by Davide Pesavento about 9 years ago
- Status changed from Code review to Closed