Project

General

Profile

Actions

Bug #2333

closed

Signal: disconnecting a handler while it is executing is unsafe

Added by Davide Pesavento over 9 years ago. Updated over 8 years ago.

Status:
Closed
Priority:
Normal
Category:
Utils
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:

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.


Related issues 2 (0 open2 closed)

Related to ndn-cxx - Bug #2302: UtilSignal/DisconnectSelfInHandler segfaultClosedJunxiao Shi

Actions
Related to ndn-cxx - Bug #2523: UtilSignal/DisconnectSelfInHandler use-after-freeClosedJunxiao Shi

Actions
Actions #1

Updated by Davide Pesavento over 9 years ago

  • Related to Bug #2302: UtilSignal/DisconnectSelfInHandler segfault added
Actions #2

Updated by Davide Pesavento about 9 years ago

  • Related to Bug #2523: UtilSignal/DisconnectSelfInHandler use-after-free added
Actions #3

Updated by Davide Pesavento over 8 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.

Actions #4

Updated by Junxiao Shi over 8 years ago

  • Assignee set to Davide Pesavento

20151022 conference call approves Davide's solution.

Actions #5

Updated by Davide Pesavento over 8 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.

Actions #6

Updated by Junxiao Shi over 8 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 Connections become invalid immediately.

Actions #7

Updated by Davide Pesavento over 8 years ago

Makes sense.

Actions #8

Updated by Davide Pesavento over 8 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 30 to 100
Actions #9

Updated by Davide Pesavento over 8 years ago

  • Status changed from Code review to Closed
Actions

Also available in: Atom PDF