From 634d325e5d823f65226e16972ea15de6eaee32ca Mon Sep 17 00:00:00 2001 From: Robin Gareus Date: Thu, 4 Feb 2021 21:04:40 +0100 Subject: [PATCH] Prevent deadlock when disconnecting The backend holds `_port_callback_mutex` while disconnecting ports. In some cases disconnecting a port can drop the last reference resulting in a port-deletion from the connection handler. This in turn will eventually aquire the `_port_callback_mutex` and deadlock. This is now circumvented by using atomic operations instead of taking a lock to set the `_port_change_flag`. The flag is also used to trigger a latency update in some cases, atomic is preferable to taking a lock to set this flag. -- Full bt: https://paste.debian.net/1184056/ Short: #1 in pthread_mutex_lock () #2 in ARDOUR::PortEngineSharedImpl::port_connect_add_remove_callback() #3 in ARDOUR::BackendPort::~BackendPort() #4 in ARDOUR::DummyPort::~DummyPort() #6 in ARDOUR::DummyAudioPort::~DummyAudioPort() #7 in boost::checked_delete(ARDOUR::BackendPort*) #12 in boost::shared_ptr::reset() #13 in ARDOUR::Port::drop() #14 in ARDOUR::Port::~Port() #15 in ARDOUR::AudioPort::~AudioPort() #17 in ARDOUR::AudioEngine::add_pending_port_deletion(ARDOUR::Port*) #20 in boost::detail::sp_counted_base::release() #37 in ARDOUR::PortManager::connect_callback() at libs/ardour/port_manager.cc:788 #38 in ARDOUR::DummyAudioBackend::main_process_thread() at libs/backends/dummy/dummy_audiobackend.cc:1018 --- libs/ardour/ardour/port_engine_shared.h | 7 +++---- libs/ardour/port_engine_shared.cc | 4 ++-- libs/backends/alsa/alsa_audiobackend.cc | 5 ++--- libs/backends/coreaudio/coreaudio_backend.cc | 15 +++++++-------- libs/backends/dummy/dummy_audiobackend.cc | 5 ++--- libs/backends/portaudio/portaudio_backend.cc | 7 ++----- libs/backends/pulseaudio/pulseaudio_backend.cc | 7 +++---- 7 files changed, 21 insertions(+), 29 deletions(-) diff --git a/libs/ardour/ardour/port_engine_shared.h b/libs/ardour/ardour/port_engine_shared.h index 073f309a96..00c8b88ac0 100644 --- a/libs/ardour/ardour/port_engine_shared.h +++ b/libs/ardour/ardour/port_engine_shared.h @@ -183,7 +183,8 @@ protected: std::vector _port_connection_queue; pthread_mutex_t _port_callback_mutex; - bool _port_change_flag; + + gint _port_change_flag; /* atomic */ void port_connect_callback (const std::string& a, const std::string& b, bool conn) { pthread_mutex_lock (&_port_callback_mutex); @@ -192,9 +193,7 @@ protected: } void port_connect_add_remove_callback () { - pthread_mutex_lock (&_port_callback_mutex); - _port_change_flag = true; - pthread_mutex_unlock (&_port_callback_mutex); + g_atomic_int_set (&_port_change_flag, 1); } virtual void update_system_port_latencies (); diff --git a/libs/ardour/port_engine_shared.cc b/libs/ardour/port_engine_shared.cc index 0baf23ee2d..c9937c7f61 100644 --- a/libs/ardour/port_engine_shared.cc +++ b/libs/ardour/port_engine_shared.cc @@ -190,7 +190,7 @@ BackendPort::update_connected_latency (bool for_playback) PortEngineSharedImpl::PortEngineSharedImpl (PortManager& mgr, std::string const & str) : _instance_name (str) - , _port_change_flag (false) + , _port_change_flag (0) , _portmap (new PortMap) , _ports (new PortIndex) { @@ -435,8 +435,8 @@ PortEngineSharedImpl::clear_ports () _ports.flush (); _portmap.flush (); + g_atomic_int_set (&_port_change_flag, 0); pthread_mutex_lock (&_port_callback_mutex); - _port_change_flag = false; _port_connection_queue.clear(); pthread_mutex_unlock (&_port_callback_mutex); } diff --git a/libs/backends/alsa/alsa_audiobackend.cc b/libs/backends/alsa/alsa_audiobackend.cc index ae8298a123..26eb6011c0 100644 --- a/libs/backends/alsa/alsa_audiobackend.cc +++ b/libs/backends/alsa/alsa_audiobackend.cc @@ -984,7 +984,7 @@ AlsaAudioBackend::_start (bool for_latency_measurement) engine.reconnect_ports (); _run = true; - _port_change_flag = false; + g_atomic_int_set (&_port_change_flag, 0); if (pbd_realtime_pthread_create (PBD_SCHED_FIFO, PBD_RT_PRI_MAIN, PBD_RT_STACKSIZE_PROC, &_main_thread, pthread_process, this)) @@ -2039,9 +2039,8 @@ AlsaAudioBackend::main_process_thread () bool connections_changed = false; bool ports_changed = false; if (!pthread_mutex_trylock (&_port_callback_mutex)) { - if (_port_change_flag) { + if (g_atomic_int_compare_and_exchange (&_port_change_flag, 1, 0)) { ports_changed = true; - _port_change_flag = false; } if (!_port_connection_queue.empty ()) { connections_changed = true; diff --git a/libs/backends/coreaudio/coreaudio_backend.cc b/libs/backends/coreaudio/coreaudio_backend.cc index 246e9e7c48..20aeef9ff6 100644 --- a/libs/backends/coreaudio/coreaudio_backend.cc +++ b/libs/backends/coreaudio/coreaudio_backend.cc @@ -640,7 +640,7 @@ CoreAudioBackend::_start (bool for_latency_measurement) _preinit = true; _run = true; - _port_change_flag = false; + g_atomic_int_set (&_port_change_flag, 0); if (_midi_driver_option == _("CoreMidi")) { _midiio->set_enabled(true); @@ -698,7 +698,7 @@ CoreAudioBackend::_start (bool for_latency_measurement) engine.reconnect_ports (); // force an initial registration_callback() & latency re-compute - _port_change_flag = true; + g_atomic_int_set (&_port_change_flag, 1); pre_process (); _dsp_load_calc.reset (); @@ -988,7 +988,7 @@ CoreAudioBackend::coremidi_rediscover() #ifndef NDEBUG printf("unregister MIDI Output: %s\n", (*it)->name().c_str()); #endif - _port_change_flag = true; + g_atomic_int_set (&_port_change_flag, 1); unregister_port((*it)); it = _system_midi_out.erase(it); } @@ -1008,7 +1008,7 @@ CoreAudioBackend::coremidi_rediscover() #ifndef NDEBUG printf("unregister MIDI Input: %s\n", (*it)->name().c_str()); #endif - _port_change_flag = true; + g_atomic_int_set (&_port_change_flag, 1); unregister_port((*it)); it = _system_midi_in.erase(it); } @@ -1034,7 +1034,7 @@ CoreAudioBackend::coremidi_rediscover() BackendPortPtr pp = boost::dynamic_pointer_cast(p); pp->set_hw_port_name(_midiio->port_name(i, true)); _system_midi_in.push_back(pp); - _port_change_flag = true; + g_atomic_int_set (&_port_change_flag, 1); } for (size_t i = 0; i < _midiio->n_midi_outputs(); ++i) { @@ -1057,7 +1057,7 @@ CoreAudioBackend::coremidi_rediscover() BackendPortPtr pp = boost::dynamic_pointer_cast(p); pp->set_hw_port_name(_midiio->port_name(i, false)); _system_midi_out.push_back(pp); - _port_change_flag = true; + g_atomic_int_set (&_port_change_flag, 1); } @@ -1229,9 +1229,8 @@ CoreAudioBackend::pre_process () bool connections_changed = false; bool ports_changed = false; if (!pthread_mutex_trylock (&_port_callback_mutex)) { - if (_port_change_flag) { + if (g_atomic_int_compare_and_exchange (&_port_change_flag, 1, 0)) { ports_changed = true; - _port_change_flag = false; } if (!_port_connection_queue.empty ()) { connections_changed = true; diff --git a/libs/backends/dummy/dummy_audiobackend.cc b/libs/backends/dummy/dummy_audiobackend.cc index 18aab7f5b9..6eb67aad18 100644 --- a/libs/backends/dummy/dummy_audiobackend.cc +++ b/libs/backends/dummy/dummy_audiobackend.cc @@ -460,7 +460,7 @@ DummyAudioBackend::_start (bool /*for_latency_measurement*/) } engine.reconnect_ports (); - _port_change_flag = false; + g_atomic_int_set (&_port_change_flag, 0); if (pbd_pthread_create (PBD_RT_STACKSIZE_PROC, &_main_thread, pthread_process, this)) { PBD::error << _("DummyAudioBackend: cannot start.") << endmsg; @@ -1006,9 +1006,8 @@ DummyAudioBackend::main_process_thread () bool connections_changed = false; bool ports_changed = false; if (!pthread_mutex_trylock (&_port_callback_mutex)) { - if (_port_change_flag) { + if (g_atomic_int_compare_and_exchange (&_port_change_flag, 1, 0)) { ports_changed = true; - _port_change_flag = false; } if (!_port_connection_queue.empty ()) { connections_changed = true; diff --git a/libs/backends/portaudio/portaudio_backend.cc b/libs/backends/portaudio/portaudio_backend.cc index 22d8979e2d..fcfaa11914 100644 --- a/libs/backends/portaudio/portaudio_backend.cc +++ b/libs/backends/portaudio/portaudio_backend.cc @@ -636,8 +636,6 @@ PortAudioBackend::_start (bool for_latency_measurement) _measure_latency = for_latency_measurement; - _port_change_flag = false; - if (_midi_driver_option == winmme_driver_name) { _midiio->set_enabled(true); //_midiio->set_port_changed_callback(midi_port_change, this); @@ -674,7 +672,7 @@ PortAudioBackend::_start (bool for_latency_measurement) _run = true; engine.reconnect_ports (); - _port_change_flag = false; + g_atomic_int_set (&_port_change_flag, 0); _dsp_calc.reset (); @@ -1728,9 +1726,8 @@ PortAudioBackend::process_port_connection_changes () bool connections_changed = false; bool ports_changed = false; if (!pthread_mutex_trylock (&_port_callback_mutex)) { - if (_port_change_flag) { + if (g_atomic_int_compare_and_exchange (&_port_change_flag, 1, 0)) { ports_changed = true; - _port_change_flag = false; } if (!_port_connection_queue.empty ()) { connections_changed = true; diff --git a/libs/backends/pulseaudio/pulseaudio_backend.cc b/libs/backends/pulseaudio/pulseaudio_backend.cc index 693f22bad6..5d9ac8e7ac 100644 --- a/libs/backends/pulseaudio/pulseaudio_backend.cc +++ b/libs/backends/pulseaudio/pulseaudio_backend.cc @@ -625,7 +625,7 @@ PulseAudioBackend::_start (bool /*for_latency_measurement*/) engine.reconnect_ports (); _run = true; - _port_change_flag = false; + g_atomic_int_set (&_port_change_flag, 0); if (pbd_realtime_pthread_create (PBD_SCHED_FIFO, PBD_RT_PRI_MAIN, PBD_RT_STACKSIZE_PROC, &_main_thread, pthread_process, this)) { @@ -1104,9 +1104,8 @@ PulseAudioBackend::main_process_thread () bool connections_changed = false; bool ports_changed = false; if (!pthread_mutex_trylock (&_port_callback_mutex)) { - if (_port_change_flag) { - ports_changed = true; - _port_change_flag = false; + if (g_atomic_int_compare_and_exchange (&_port_change_flag, 1, 0)) { + ports_changed = true; } if (!_port_connection_queue.empty ()) { connections_changed = true;