Merge pull request #382 from FluidSynth/unregister

fix double free when deleting fluid_seqbind_t

doc/fluidsynth-v11-devdoc.txt

@@ -103,6 +103,7 @@ Changes in FluidSynth 2.0.0 concerning developers:
 - all public \c fluid_settings_* functions that return an integer which is not meant to be interpreted as bool consistently return either FLUID_OK or FLUID_FAILED
 - all public delete_* functions return void and are safe when called with NULL
 - all public functions consistently receive signed integers for soundfont ids, bank and program numbers
+- explicit client unregistering is required for fluid_sequencer_register_client() and fluid_sequencer_register_fluidsynth()
 - the shell command handler was decoupled internally, as a consequence the param list of new_fluid_server() and new_fluid_cmd_handler() was adapted
 - reverb: roomsize is now limited to an upper threshold of 1.0 to avoid exponential volume increase
 - use unique device names for the "audio.portaudio.device" setting

src/midi/fluid_seq.c

@@ -260,6 +260,9 @@ void fluid_seq_dotrace(fluid_sequencer_t* seq, char *fmt, ...) {}
  *
  * Clients can be sources or destinations of events.  Sources don't need to
  * register a callback.
+ * 
+ * @note The user must explicitly unregister any registered client with fluid_sequencer_unregister_client()
+ * before deleting the sequencer!
  */
 fluid_seq_id_t
 fluid_sequencer_register_client (fluid_sequencer_t* seq, const char *name,
@@ -302,36 +305,23 @@ void
 fluid_sequencer_unregister_client (fluid_sequencer_t* seq, fluid_seq_id_t id)
 {
 	fluid_list_t *tmp;
-	fluid_event_t* evt;
 
 	if (seq->clients == NULL) return;
 
-	evt = new_fluid_event();
-	if (evt != NULL) {
-		fluid_event_unregistering(evt);
-		fluid_event_set_dest(evt, id);
-	}
-
 	tmp = seq->clients;
 	while (tmp) {
   		fluid_sequencer_client_t *client = (fluid_sequencer_client_t*)tmp->data;
 
   		if (client->id == id) {
-			/* What should we really do if evt is null due to out-of-memory? */
-			if (client->callback != NULL && evt != NULL)
-				(client->callback)(fluid_sequencer_get_tick(seq),
-						 evt, seq, client->data);
    			if (client->name)
 				FLUID_FREE(client->name);
 			seq->clients = fluid_list_remove_link(seq->clients, tmp);
 			delete1_fluid_list(tmp);
 			FLUID_FREE(client);
-			delete_fluid_event(evt);
 			return;
   		}
    		tmp = tmp->next;
 	}
-	delete_fluid_event(evt);
 	return;
 }
 

src/midi/fluid_seqbind.c

@@ -71,6 +71,27 @@ delete_fluid_seqbind(fluid_seqbind_t* seqbind)
 /** 
  * Registers a synthesizer as a destination client of the given sequencer.
  * The \a synth is registered with the name "fluidsynth".
+ * 
+ * @warning Due to internal memory allocation, the user must explicitly unregister
+ * the client by sending a fluid_event_unregistering(). Otherwise the behaviour is 
+ * undefined after either \p seq or \p synth is destroyed.
+@code{.cpp}
+fluid_seq_id_t seqid = fluid_sequencer_register_fluidsynth(seq, synth);
+
+// ... do work
+
+fluid_event_t* evt = new_fluid_event();
+fluid_event_set_source(evt, -1);
+fluid_event_set_dest(evt, seqid);
+fluid_event_unregistering(evt);
+
+// unregister the "fluidsynth" client immediately
+fluid_sequencer_send_now(seq, evt);
+delete_fluid_event(evt);
+delete_fluid_synth(synth);
+delete_fluid_sequencer(seq);
+@endcode
+ * 
  * @param seq Sequencer instance
  * @param synth Synthesizer instance
  * @returns Sequencer client ID, or #FLUID_FAILED on error.
@@ -230,7 +251,6 @@ fluid_seq_fluidsynth_callback(unsigned int time, fluid_event_t* evt, fluid_seque
 	break;
 
   case FLUID_SEQ_UNREGISTERING: /* free ourselves */
-    seqbind->client_id = -1; /* avoid recursive call to fluid_sequencer_unregister_client */
     delete_fluid_seqbind(seqbind);
 	break;
 

src/synth/fluid_synth.c

@@ -483,17 +483,19 @@ struct _fluid_sample_timer_t
  */
 void fluid_sample_timer_process(fluid_synth_t* synth)
 {
-	fluid_sample_timer_t* st;
+	fluid_sample_timer_t* st, *stnext;
 	long msec;
 	int cont;
         unsigned int ticks = fluid_synth_get_ticks(synth);
 	
-	for (st=synth->sample_timers; st; st=st->next) {
+	for (st=synth->sample_timers; st; st=stnext) {
 		if (st->isfinished) {
 			continue;
 		}
 
 		msec = (long) (1000.0*((double) (ticks - st->starttick))/synth->sample_rate);
+		/* st may be freed in the callback below. cache it's successor now to avoid use after free */
+		stnext = st->next;
 		cont = (*st->callback)(st->data, msec);
 		if (cont == 0) {
 			st->isfinished = 1;