From 010660407323806c2d09253a38a7c72e8d31352d Mon Sep 17 00:00:00 2001 From: Jonathan Moore Liles Date: Wed, 2 Dec 2020 20:13:44 -0800 Subject: [PATCH] Mixer: Fix crash when closing project containing certain configurations of modules. --- mixer/src/Chain.C | 21 ++++++++++---- mixer/src/Chain.H | 2 +- mixer/src/Controller_Module.C | 2 -- mixer/src/Group.C | 37 ++++++++++++++++++------ mixer/src/Group.H | 2 ++ mixer/src/Mixer.C | 2 -- mixer/src/Mixer_Strip.C | 24 ++++++++++++++-- mixer/src/Mixer_Strip.H | 3 +- mixer/src/Module.C | 53 ++++++++++++++++++++++++----------- mixer/src/Module.H | 21 +++++--------- 10 files changed, 114 insertions(+), 53 deletions(-) diff --git a/mixer/src/Chain.C b/mixer/src/Chain.C index d0b173a..015c925 100644 --- a/mixer/src/Chain.C +++ b/mixer/src/Chain.C @@ -174,7 +174,9 @@ Chain::~Chain ( ) _deleting = true; - client()->lock(); + /* FIXME: client may already be dead during teardown if group is destroyed first. */ + if ( client() ) + client()->lock(); for ( unsigned int i = scratch_port.size(); i--; ) free( (sample_t*)scratch_port[i].buffer() ); @@ -182,9 +184,12 @@ Chain::~Chain ( ) /* if we leave this up to FLTK, it will happen after we've already destroyed the client */ modules_pack->clear(); + modules_pack = NULL; controls_pack->clear(); + modules_pack = NULL; - client()->unlock(); + if ( client() ) + client()->unlock(); } Group * @@ -534,6 +539,8 @@ Chain::name ( const char *name ) if ( strip()->group() ) { if ( strip()->group()->single() ) + /* we are the owner of this group and its only member, so + * rename it */ strip()->group()->name(name); } @@ -713,6 +720,8 @@ Chain::add_to_process_queue ( Module *m ) void Chain::build_process_queue ( void ) { + client()->lock(); + process_queue.clear(); for ( int i = 0; i < modules(); ++i ) @@ -780,6 +789,8 @@ Chain::build_process_queue ( void ) /* delete[] s; */ /* } */ /* } */ + + client()->unlock(); } void @@ -901,9 +912,9 @@ Chain::process ( nframes_t nframes ) { for ( std::list::const_iterator i = process_queue.begin(); i != process_queue.end(); ++i ) { - Module *m = *i; - - m->process( nframes ); + Module *m = *i; + + m->process( nframes ); } } diff --git a/mixer/src/Chain.H b/mixer/src/Chain.H index c11cdad..4843bb1 100644 --- a/mixer/src/Chain.H +++ b/mixer/src/Chain.H @@ -109,7 +109,7 @@ public: bool can_support_input_channels ( int n ); - int modules ( void ) const { return modules_pack->children(); } + int modules ( void ) const { return modules_pack ? modules_pack->children() : 0; } Module *module ( int n ) const { return (Module*)modules_pack->child( n ); } void remove ( Controller_Module *m ); void remove ( Module *m ); diff --git a/mixer/src/Controller_Module.C b/mixer/src/Controller_Module.C index 74eaca9..01a5199 100644 --- a/mixer/src/Controller_Module.C +++ b/mixer/src/Controller_Module.C @@ -96,8 +96,6 @@ Controller_Module::~Controller_Module ( ) /* shutdown JACK port, if we have one */ mode( GUI ); - -// disconnect(); } void diff --git a/mixer/src/Group.C b/mixer/src/Group.C index 1f57642..9052a47 100644 --- a/mixer/src/Group.C +++ b/mixer/src/Group.C @@ -56,9 +56,18 @@ Group::~Group ( ) { DMESSAGE( "Destroying group" ); + for ( std::list::iterator i = strips.begin(); + i != strips.end(); + i++ ) + { + /* avoid a use after free during project close when the group + * may be destroyed before its member strips are */ + (*i)->clear_group(); + } + if ( _name ) free( _name ); - + deactivate(); } @@ -101,12 +110,16 @@ Group::set ( Log_Entry &e ) void Group::latency ( jack_latency_callback_mode_t mode ) { - for ( std::list::iterator i = strips.begin(); - i != strips.end(); - i++ ) + if ( trylock() ) { - if ( (*i)->chain() ) - (*i)->chain()->set_latency(mode == JackCaptureLatency ? JACK::Port::Input : JACK::Port::Output ); + for ( std::list::iterator i = strips.begin(); + i != strips.end(); + i++ ) + { + if ( (*i)->chain() ) + (*i)->chain()->set_latency(mode == JackCaptureLatency ? JACK::Port::Input : JACK::Port::Output ); + } + unlock(); } } @@ -208,8 +221,9 @@ Group::process ( nframes_t nframes ) void Group::recal_load_coef ( void ) { - _load_coef = 1.0f / ( nframes() / (float)sample_rate() * 1000000.0 ); + _load_coef = 1.0f / ( nframes() / (float)sample_rate() * 1000000.0f ); } + int Group::sample_rate_changed ( nframes_t srate ) { @@ -273,6 +287,7 @@ void Group::add ( Mixer_Strip *o ) { lock(); + if ( ! active() ) { /* to call init */ @@ -280,10 +295,12 @@ Group::add ( Mixer_Strip *o ) name(n); free(n); } + if ( o->chain() ) o->chain()->thaw_ports(); strips.push_back(o); + unlock(); } @@ -291,13 +308,15 @@ void Group::remove ( Mixer_Strip *o ) { lock(); + strips.remove(o); + if ( o->chain() ) o->chain()->freeze_ports(); + if ( strips.size() == 0 && active() ) - { Client::close(); - } + unlock(); } diff --git a/mixer/src/Group.H b/mixer/src/Group.H index 5d3dd3d..84c7a03 100644 --- a/mixer/src/Group.H +++ b/mixer/src/Group.H @@ -97,6 +97,8 @@ public: void add (Mixer_Strip*); void remove (Mixer_Strip*); + int children ( void ) const { return strips.size(); } + /* Engine *engine ( void ) { return _engine; } */ }; diff --git a/mixer/src/Mixer.C b/mixer/src/Mixer.C index 0969a9e..6d859f8 100644 --- a/mixer/src/Mixer.C +++ b/mixer/src/Mixer.C @@ -821,8 +821,6 @@ void Mixer::remove ( Mixer_Strip *ms ) mixer_strips->remove( ms ); - ms->group()->remove( ms ); - if ( parent() ) parent()->redraw(); } diff --git a/mixer/src/Mixer_Strip.C b/mixer/src/Mixer_Strip.C index 88dffa0..54c3d62 100644 --- a/mixer/src/Mixer_Strip.C +++ b/mixer/src/Mixer_Strip.C @@ -73,6 +73,7 @@ Mixer_Strip::Mixer_Strip( const char *strip_name ) : Fl_Group( 0, 0, 120, 600 ) init(); + /* create single member group */ _group = new Group(strip_name, true); _group->add( this ); @@ -105,14 +106,30 @@ Mixer_Strip::~Mixer_Strip ( ) // _chain->engine()->lock(); log_destroy(); - + mixer->remove( this ); /* make sure this gets destroyed before the chain */ fader_tab->clear(); - delete _chain; - _chain = NULL; + if ( _group ) + { + _group->remove( this ); + } + + if ( _chain ) + { + delete _chain; + _chain = NULL; + } + + if ( _group ) + { + if ( 0 == _group->children() ) + delete _group; + + _group = NULL; + } } @@ -347,6 +364,7 @@ void Mixer_Strip::cb_handle(Fl_Widget* o, void* v) { void Mixer_Strip::group ( Group *g ) { + /* FIXME: what is the intention here? */ if ( !g && _group && _group->single() ) return; diff --git a/mixer/src/Mixer_Strip.H b/mixer/src/Mixer_Strip.H index ce7967a..081d207 100644 --- a/mixer/src/Mixer_Strip.H +++ b/mixer/src/Mixer_Strip.H @@ -169,7 +169,8 @@ public: Controller_Module *spatializer ( void ); - Group *group ( void ) { return _group;} + Group *group ( void ) { return _group; } + void clear_group ( void ) { _group = NULL; } // int group ( void ) const; void group ( Group * ); diff --git a/mixer/src/Module.C b/mixer/src/Module.C index ea712d3..f41a92a 100644 --- a/mixer/src/Module.C +++ b/mixer/src/Module.C @@ -104,27 +104,36 @@ Module::~Module ( ) if ( control_input[i].connected() ) { Module *o = (Module*)control_input[i].connected_port()->module(); - - if ( ! o->is_default() ) - { - control_input[i].disconnect(); - - DMESSAGE( "Deleting connected module %s", o->label() ); - - delete o; - } - else - { - control_input[i].disconnect(); - } - - } + if ( !o ) + { + DWARNING( "Programming error. Connected port has null module. %s %s", + + label(), + control_input[i].connected_port()->name()); + } + + control_input[i].disconnect(); + + if ( o ) + { + if ( ! o->is_default() ) + { + DMESSAGE( "Deleting connected module %s", o->label() ); + + delete o; + } + } + } + control_input[i].destroy_osc_port(); } for ( unsigned int i = 0; i < control_output.size(); ++i ) control_output[i].disconnect(); + aux_audio_output.clear(); + aux_audio_input.clear(); + audio_input.clear(); audio_output.clear(); @@ -1180,7 +1189,19 @@ Module::freeze_ports ( void ) for ( int i = 0; i < ncontrol_inputs(); ++i ) { if ( control_input[i].connected() ) - control_input[i].connected_port()->module()->freeze_ports(); + { + if ( ! control_input[i].connected_port()->module() ) + { + DWARNING( "Programming error. Connected port has null module. %s %s", + + name(), + control_input[i].connected_port()->name()); + } + else + { + control_input[i].connected_port()->module()->freeze_ports(); + } + } } for ( unsigned int i = 0; i < aux_audio_input.size(); ++i ) diff --git a/mixer/src/Module.H b/mixer/src/Module.H index 8eebc82..7258244 100644 --- a/mixer/src/Module.H +++ b/mixer/src/Module.H @@ -173,6 +173,9 @@ public: virtual ~Port ( ) { + /* FIXME: will this cause problems with cloning an instance? */ + disconnect(); + if ( _by_number_path ) free( _by_number_path ); _by_number_path = NULL; @@ -321,20 +324,10 @@ public: /* disconnect from *all* connected ports */ void disconnect ( void ) { - if ( _connected.size() ) - { - for ( std::list::iterator i = _connected.begin(); i != _connected.end(); i++ ) - { - Port *p = *i; - - /* iterator about to be invalidated... */ - i = _connected.erase(i); - - disconnect(p); - } - } - } - + while ( _connected.size() ) + disconnect(_connected.front()); + } + void jack_port ( JACK::Port *v ) { _jack_port = v; } JACK::Port *jack_port ( void ) const { return _jack_port; }