From 4ac3134c050ba28b57e0ae9686eb1f6d83e6d586 Mon Sep 17 00:00:00 2001 From: Andre Noll Date: Mon, 9 Apr 2012 21:57:54 +0200 Subject: [PATCH] btr: Remove btr_free_node(). This has turned out to be source for bugs. Deallocate everything in btr_remove_node() hence making removing the node and freeing its resources an atomic operation. To avoid dangling pointers to freed btrn nodes, the argument of btr_remove_node() is changed to to struct btr_node **btrnp. --- aacdec_filter.c | 2 +- alsa_write.c | 2 +- amp_filter.c | 2 +- ao_write.c | 24 ++++++++++-------------- audioc.c | 4 +--- audiod.c | 20 ++++++++++---------- buffer_tree.c | 35 ++++++++++++++--------------------- buffer_tree.h | 3 +-- client.c | 8 ++++---- client_common.c | 5 ++--- compress_filter.c | 2 +- dccp_recv.c | 2 +- fecdec_filter.c | 2 +- file_write.c | 2 +- filter.c | 6 +++--- flacdec_filter.c | 2 +- grab_client.c | 4 +--- http_recv.c | 2 +- interactive.c | 4 +--- mp3dec_filter.c | 2 +- oggdec_filter.c | 2 +- oss_write.c | 2 +- osx_write.c | 5 ++--- recv.c | 5 +++-- spxdec_filter.c | 2 +- stdin.c | 2 +- stdout.c | 2 +- udp_recv.c | 2 +- wav_filter.c | 2 +- wmadec_filter.c | 2 +- write.c | 6 +++--- 31 files changed, 73 insertions(+), 92 deletions(-) diff --git a/aacdec_filter.c b/aacdec_filter.c index a4414e8f..c8b60925 100644 --- a/aacdec_filter.c +++ b/aacdec_filter.c @@ -204,7 +204,7 @@ out: err: assert(ret < 0); t->error = ret; - btr_remove_node(btrn); + btr_remove_node(&fn->btrn); } /** diff --git a/alsa_write.c b/alsa_write.c index f50ad6a3..535e7194 100644 --- a/alsa_write.c +++ b/alsa_write.c @@ -324,7 +324,7 @@ again: ret = -E_ALSA; err: assert(ret < 0); - btr_remove_node(btrn); + btr_remove_node(&wn->btrn); t->error = ret; } diff --git a/amp_filter.c b/amp_filter.c index 3dc1e411..7e88cc49 100644 --- a/amp_filter.c +++ b/amp_filter.c @@ -118,7 +118,7 @@ next_buffer: err: assert(ret < 0); t->error = ret; - btr_remove_node(btrn); + btr_remove_node(&fn->btrn); } static void amp_free_config(void *conf) diff --git a/ao_write.c b/ao_write.c index 93861ab6..a45d4fb9 100644 --- a/ao_write.c +++ b/ao_write.c @@ -280,7 +280,6 @@ static void aow_post_select(__a_unused struct sched *s, struct task *t) { struct writer_node *wn = container_of(t, struct writer_node, task); - struct btr_node *btrn = wn->btrn; struct private_aow_data *pawd = wn->private_data; int ret; @@ -288,14 +287,14 @@ static void aow_post_select(__a_unused struct sched *s, int32_t rate, ch, format; struct btr_node_description bnd; - ret = btr_node_status(btrn, wn->min_iqs, BTR_NT_LEAF); + ret = btr_node_status(wn->btrn, wn->min_iqs, BTR_NT_LEAF); if (ret < 0) goto remove_btrn; if (ret == 0) return; - get_btr_sample_rate(btrn, &rate); - get_btr_channels(btrn, &ch); - get_btr_sample_format(btrn, &format); + get_btr_sample_rate(wn->btrn, &rate); + get_btr_channels(wn->btrn, &ch); + get_btr_sample_format(wn->btrn, &format); ret = aow_init(wn, rate, ch, format); if (ret < 0) goto remove_btrn; @@ -303,7 +302,7 @@ static void aow_post_select(__a_unused struct sched *s, /* set up thread btr node */ bnd.name = "ao_thread_btrn"; - bnd.parent = btrn; + bnd.parent = wn->btrn; bnd.child = NULL; bnd.handler = NULL; bnd.context = pawd; @@ -316,27 +315,24 @@ static void aow_post_select(__a_unused struct sched *s, return; } pthread_mutex_lock(&pawd->mutex); - ret = btr_node_status(btrn, wn->min_iqs, BTR_NT_LEAF); + ret = btr_node_status(wn->btrn, wn->min_iqs, BTR_NT_LEAF); if (ret > 0) { - btr_pushdown(btrn); + btr_pushdown(wn->btrn); pthread_cond_signal(&pawd->data_available); } pthread_mutex_unlock(&pawd->mutex); if (ret >= 0) goto out; pthread_mutex_lock(&pawd->mutex); - btr_remove_node(btrn); - btrn = NULL; + btr_remove_node(&wn->btrn); PARA_INFO_LOG("waiting for thread to terminate\n"); pthread_cond_signal(&pawd->data_available); pthread_mutex_unlock(&pawd->mutex); pthread_join(pawd->thread, NULL); remove_thread_btrn: - btr_remove_node(pawd->thread_btrn); - btr_free_node(pawd->thread_btrn); + btr_remove_node(&pawd->thread_btrn); remove_btrn: - if (btrn) - btr_remove_node(btrn); + btr_remove_node(&wn->btrn); out: t->error = ret; } diff --git a/audioc.c b/audioc.c index d3e092e2..3997001d 100644 --- a/audioc.c +++ b/audioc.c @@ -128,9 +128,7 @@ static void audioc_post_select(struct sched *s, struct task *t) out: if (ret < 0) { free(buf); - btr_remove_node(at->btrn); - btr_free_node(at->btrn); - at->btrn = NULL; + btr_remove_node(&at->btrn); close(at->fd); } t->error = ret; diff --git a/audiod.c b/audiod.c index 93bc8da3..9f2aa5ab 100644 --- a/audiod.c +++ b/audiod.c @@ -381,7 +381,7 @@ static void close_receiver(int slot_num) PARA_NOTICE_LOG("closing %s receiver in slot %d\n", audio_formats[s->format], slot_num); a->receiver->close(s->receiver_node); - btr_free_node(s->receiver_node->btrn); + btr_remove_node(&s->receiver_node->btrn); free(s->receiver_node); s->receiver_node = NULL; tv_add(now, &(struct timeval)EMBRACE(0, 200 * 1000), @@ -397,7 +397,7 @@ static void writer_cleanup(struct writer_node *wn) w = writers + wn->writer_num; PARA_INFO_LOG("closing %s\n", writer_names[wn->writer_num]); w->close(wn); - btr_free_node(wn->btrn); + btr_remove_node(&wn->btrn); } static void close_writers(struct slot_info *s) @@ -434,7 +434,7 @@ static void close_filters(struct slot_info *s) f = filters + fn->filter_num; if (f->close) f->close(fn); - btr_free_node(fn->btrn); + btr_remove_node(&fn->btrn); } free(s->fns); s->fns = NULL; @@ -446,12 +446,12 @@ static void close_filters(struct slot_info *s) * task. Note that the scheduler checks t->error also _before_ each pre/post * select call, so the victim will never be scheduled again. */ -static void kill_btrn(struct btr_node *btrn, struct task *t, int error) +static void kill_btrn(struct btr_node **btrnp, struct task *t, int error) { if (t->error < 0) return; t->error = error; - btr_remove_node(btrn); + btr_remove_node(btrnp); } static void kill_all_decoders(int error) @@ -464,7 +464,7 @@ static void kill_all_decoders(int error) continue; if (!s->receiver_node) continue; - kill_btrn(s->receiver_node->btrn, &s->receiver_node->task, + kill_btrn(&s->receiver_node->btrn, &s->receiver_node->task, error); } } @@ -563,7 +563,7 @@ static int open_receiver(int format) EMBRACE(.name = r->name, .context = rn)); ret = r->open(rn); if (ret < 0) { - btr_free_node(rn->btrn); + btr_remove_node(&rn->btrn); free(rn); return ret; } @@ -1196,7 +1196,7 @@ static void status_post_select(struct sched *s, struct task *t) if (!st->ct) goto out; if (st->ct->task.error >= 0) { - kill_btrn(st->ct->btrn, &st->ct->task, -E_AUDIOD_OFF); + kill_btrn(&st->ct->btrn, &st->ct->task, -E_AUDIOD_OFF); goto out; } close_stat_pipe(); @@ -1218,7 +1218,7 @@ static void status_post_select(struct sched *s, struct task *t) struct timeval diff; tv_diff(now, &st->last_status_read, &diff); if (diff.tv_sec > 61) - kill_btrn(st->ct->btrn, &st->ct->task, + kill_btrn(&st->ct->btrn, &st->ct->task, -E_STATUS_TIMEOUT); goto out; } @@ -1226,7 +1226,7 @@ static void status_post_select(struct sched *s, struct task *t) sz = btr_next_buffer(st->btrn, &buf); ret = for_each_stat_item(buf, sz, update_item); if (ret < 0) { - kill_btrn(st->ct->btrn, &st->ct->task, ret); + kill_btrn(&st->ct->btrn, &st->ct->task, ret); goto out; } if (sz != ret) { diff --git a/buffer_tree.c b/buffer_tree.c index 7d79636f..5c884709 100644 --- a/buffer_tree.c +++ b/buffer_tree.c @@ -729,44 +729,37 @@ void btr_drain(struct btr_node *btrn) btr_drop_buffer_reference(br); } -/** - * Free all resources allocated by btr_new_node(). - * - * \param btrn Pointer to a btr node obtained by \ref btr_new_node(). - * - * Like free(3), it is OK to call this with a \p NULL pointer argument. - */ -void btr_free_node(struct btr_node *btrn) -{ - if (!btrn) - return; - free(btrn->name); - free(btrn); -} - /** * Remove a node from a buffer tree. * - * \param btrn The node to remove. + * \param btrnp Determines the node to remove. * - * This makes all child nodes of \a btrn orphans and removes \a btrn from the - * list of children of its parent. Moreover, the input queue of \a btrn is - * flushed if it is not empty. + * This orphans all children of the node given by \a btrnp and removes this + * node from the child list of its parent. Moreover, the input queue is flushed + * and the node pointer given by \a btrp is set to \p NULL. * * \sa \ref btr_splice_out_node. */ -void btr_remove_node(struct btr_node *btrn) +void btr_remove_node(struct btr_node **btrnp) { struct btr_node *ch; + struct btr_node *btrn; - if (!btrn) + if (!btrnp) return; + btrn = *btrnp; + if (!btrn) + goto out; PARA_INFO_LOG("removing btr node %s from buffer tree\n", btrn->name); FOR_EACH_CHILD(ch, btrn) ch->parent = NULL; btr_drain(btrn); if (btrn->parent) list_del(&btrn->node); + free(btrn->name); + free(btrn); +out: + *btrnp = NULL; } /** diff --git a/buffer_tree.h b/buffer_tree.h index 4d27ec7b..91106a19 100644 --- a/buffer_tree.h +++ b/buffer_tree.h @@ -182,8 +182,7 @@ void btr_copy(const void *src, size_t n, struct btr_pool *btrp, struct btr_node *btrn); struct btr_node *btr_new_node(struct btr_node_description *bnd); -void btr_remove_node(struct btr_node *btrn); -void btr_free_node(struct btr_node *btrn); +void btr_remove_node(struct btr_node **btrnp); void btr_add_output(char *buf, size_t size, struct btr_node *btrn); size_t btr_get_input_queue_size(struct btr_node *btrn); size_t btr_get_output_queue_size(struct btr_node *btrn); diff --git a/client.c b/client.c index c194e192..6268c3de 100644 --- a/client.c +++ b/client.c @@ -114,11 +114,11 @@ static int execute_client_command(const char *cmd, char **result) goto out; schedule(&command_sched); *result = exec_task.result_buf; - btr_remove_node(exec_task.btrn); + btr_remove_node(&exec_task.btrn); client_disconnect(ct); ret = 1; out: - btr_free_node(exec_task.btrn); + btr_remove_node(&exec_task.btrn); if (ret < 0) free(exec_task.result_buf); return ret; @@ -619,7 +619,7 @@ out: if (ret < 0) PARA_ERROR_LOG("%s\n", para_strerror(-ret)); client_close(ct); - btr_free_node(sit.btrn); - btr_free_node(sot.btrn); + btr_remove_node(&sit.btrn); + btr_remove_node(&sot.btrn); return ret < 0? EXIT_FAILURE : EXIT_SUCCESS; } diff --git a/client_common.c b/client_common.c index 649a1b4f..a4aa6d8c 100644 --- a/client_common.c +++ b/client_common.c @@ -48,8 +48,7 @@ void client_disconnect(struct client_task *ct) ct->scc.recv = NULL; sc_free(ct->scc.send); ct->scc.send = NULL; - btr_free_node(ct->btrn); - ct->btrn = NULL; + btr_remove_node(&ct->btrn); } /** @@ -562,7 +561,7 @@ out: if (!ct->use_sideband && ret != -E_SERVER_EOF && ret != -E_BTR_EOF && ret != -E_EOF) PARA_ERROR_LOG("%s\n", para_strerror(-t->error)); - btr_remove_node(btrn); + btr_remove_node(&ct->btrn); } } diff --git a/compress_filter.c b/compress_filter.c index 74ea59f3..3dee5ccd 100644 --- a/compress_filter.c +++ b/compress_filter.c @@ -109,7 +109,7 @@ next_buffer: err: assert(ret < 0); t->error = ret; - btr_remove_node(btrn); + btr_remove_node(&fn->btrn); } /** TODO: Add sanity checks */ diff --git a/dccp_recv.c b/dccp_recv.c index 21c69322..ea6884b1 100644 --- a/dccp_recv.c +++ b/dccp_recv.c @@ -151,7 +151,7 @@ static void dccp_recv_post_select(struct sched *s, struct task *t) out: if (ret >= 0) return; - btr_remove_node(rn->btrn); + btr_remove_node(&rn->btrn); t->error = ret; } diff --git a/fecdec_filter.c b/fecdec_filter.c index aea32bfd..945e3e9d 100644 --- a/fecdec_filter.c +++ b/fecdec_filter.c @@ -470,7 +470,7 @@ next_buffer: out: t->error = ret; if (ret < 0) - btr_remove_node(btrn); + btr_remove_node(&fn->btrn); } static void fecdec_open(struct filter_node *fn) diff --git a/file_write.c b/file_write.c index 5c5c5149..98d15a43 100644 --- a/file_write.c +++ b/file_write.c @@ -129,7 +129,7 @@ static void file_write_post_select(__a_unused struct sched *s, btr_consume(btrn, ret); out: if (ret < 0) - btr_remove_node(btrn); + btr_remove_node(&wn->btrn); t->error = ret; } diff --git a/filter.c b/filter.c index 81258a6a..55b48ea2 100644 --- a/filter.c +++ b/filter.c @@ -161,13 +161,13 @@ out_cleanup: f = filters + fn->filter_num; if (f->close) f->close(fn); - btr_free_node(fn->btrn); + btr_remove_node(&fn->btrn); free(fn->conf); free(fn); } free(fns); - btr_free_node(sit->btrn); - btr_free_node(sot->btrn); + btr_remove_node(&sit->btrn); + btr_remove_node(&sot->btrn); out: if (ret < 0) PARA_EMERG_LOG("%s\n", para_strerror(-ret)); diff --git a/flacdec_filter.c b/flacdec_filter.c index e8baa6b4..dfd90213 100644 --- a/flacdec_filter.c +++ b/flacdec_filter.c @@ -257,7 +257,7 @@ static void flacdec_post_select(__a_unused struct sched *s, struct task *t) out: t->error = ret; if (ret < 0) - btr_remove_node(btrn); + btr_remove_node(&fn->btrn); } static void flacdec_close(struct filter_node *fn) diff --git a/grab_client.c b/grab_client.c index 07f779bd..2b8c8d3e 100644 --- a/grab_client.c +++ b/grab_client.c @@ -166,9 +166,7 @@ void activate_grab_clients(struct sched *s) static int gc_close(struct grab_client *gc, int err) { - btr_remove_node(gc->btrn); - btr_free_node(gc->btrn); - gc->btrn = NULL; + btr_remove_node(&gc->btrn); PARA_INFO_LOG("closing gc: %s\n", para_strerror(-err)); list_move(&gc->node, &inactive_grab_client_list); if (err == -E_GC_WRITE || (gc->flags & GF_ONE_SHOT)) { diff --git a/http_recv.c b/http_recv.c index b4bf1530..11602f95 100644 --- a/http_recv.c +++ b/http_recv.c @@ -128,7 +128,7 @@ static void http_recv_post_select(struct sched *s, struct task *t) out: if (ret >= 0) return; - btr_remove_node(rn->btrn); + btr_remove_node(&rn->btrn); t->error = ret; } diff --git a/interactive.c b/interactive.c index f9ea3612..68891ac1 100644 --- a/interactive.c +++ b/interactive.c @@ -298,9 +298,7 @@ static void i9e_post_select(struct sched *s, struct task *t) btr_consume(btrn, ret); goto out; rm_btrn: - btr_remove_node(btrn); - btr_free_node(btrn); - i9ep->stdout_btrn = NULL; + btr_remove_node(&i9ep->stdout_btrn); rl_set_prompt(i9ep->ici->prompt); rl_forced_update_display(); out: diff --git a/mp3dec_filter.c b/mp3dec_filter.c index 4bdbc6fd..5a177c19 100644 --- a/mp3dec_filter.c +++ b/mp3dec_filter.c @@ -166,7 +166,7 @@ decode: err: assert(ret < 0); t->error = ret; - btr_remove_node(btrn); + btr_remove_node(&fn->btrn); } static void mp3dec_open(struct filter_node *fn) diff --git a/oggdec_filter.c b/oggdec_filter.c index 9498313c..c5c0b538 100644 --- a/oggdec_filter.c +++ b/oggdec_filter.c @@ -259,7 +259,7 @@ static void ogg_post_select(__a_unused struct sched *s, struct task *t) out: t->error = ret; if (ret < 0) - btr_remove_node(btrn); + btr_remove_node(&fn->btrn); } /** diff --git a/oss_write.c b/oss_write.c index eae4167a..1d9add7b 100644 --- a/oss_write.c +++ b/oss_write.c @@ -199,7 +199,7 @@ static void oss_post_select(__a_unused struct sched *s, out: t->error = ret; if (ret < 0) - btr_remove_node(btrn); + btr_remove_node(&wn->btrn); } __malloc static void *oss_parse_config_or_die(const char *options) diff --git a/osx_write.c b/osx_write.c index 0f9d9605..2c6fd0d2 100644 --- a/osx_write.c +++ b/osx_write.c @@ -309,10 +309,9 @@ static void osx_write_post_select(__a_unused struct sched *s, struct task *t) AudioOutputUnitStop(powd->audio_unit); AudioUnitUninitialize(powd->audio_unit); CloseComponent(powd->audio_unit); - btr_remove_node(powd->callback_btrn); - btr_free_node(powd->callback_btrn); + btr_remove_node(&powd->callback_btrn); remove_btrn: - btr_remove_node(btrn); + btr_remove_node(&wn->btrn); PARA_NOTICE_LOG("%s\n", para_strerror(-ret)); out: t->error = ret; diff --git a/recv.c b/recv.c index f14ea287..c021b17b 100644 --- a/recv.c +++ b/recv.c @@ -112,10 +112,11 @@ int main(int argc, char *argv[]) out: if (r_opened) r->close(&rn); - btr_free_node(rn.btrn); - btr_free_node(sot.btrn); + btr_remove_node(&rn.btrn); + btr_remove_node(&sot.btrn); if (rn.conf) r->free_config(rn.conf); + if (ret < 0) PARA_ERROR_LOG("%s\n", para_strerror(-ret)); return ret < 0? EXIT_FAILURE : EXIT_SUCCESS; diff --git a/spxdec_filter.c b/spxdec_filter.c index e54b2657..9a827d53 100644 --- a/spxdec_filter.c +++ b/spxdec_filter.c @@ -292,7 +292,7 @@ next_buffer: fail: if (ret < 0) { t->error = ret; - btr_remove_node(btrn); + btr_remove_node(&fn->btrn); } } diff --git a/stdin.c b/stdin.c index ac1581d1..8dce76b5 100644 --- a/stdin.c +++ b/stdin.c @@ -83,7 +83,7 @@ static void stdin_post_select(struct sched *s, struct task *t) if (ret >= 0) return; err: - btr_remove_node(sit->btrn); + btr_remove_node(&sit->btrn); //btr_pool_free(sit->btrp); t->error = ret; } diff --git a/stdout.c b/stdout.c index abe7abc9..066f1af7 100644 --- a/stdout.c +++ b/stdout.c @@ -75,7 +75,7 @@ static void stdout_post_select(struct sched *s, struct task *t) } out: if (ret < 0) - btr_remove_node(btrn); + btr_remove_node(&sot->btrn); t->error = ret; } /** diff --git a/udp_recv.c b/udp_recv.c index c1f23830..615553c5 100644 --- a/udp_recv.c +++ b/udp_recv.c @@ -83,7 +83,7 @@ static void udp_recv_post_select(__a_unused struct sched *s, struct task *t) out: if (ret >= 0) return; - btr_remove_node(btrn); + btr_remove_node(&rn->btrn); t->error = ret; close(rn->fd); rn->fd = -1; diff --git a/wav_filter.c b/wav_filter.c index c5079061..e8630f9e 100644 --- a/wav_filter.c +++ b/wav_filter.c @@ -117,7 +117,7 @@ err: if (ret == -E_WAV_SUCCESS) btr_splice_out_node(btrn); else { - btr_remove_node(btrn); + btr_remove_node(&fn->btrn); PARA_ERROR_LOG("%s\n", para_strerror(-ret)); } } diff --git a/wmadec_filter.c b/wmadec_filter.c index e58754f5..a35c1742 100644 --- a/wmadec_filter.c +++ b/wmadec_filter.c @@ -1267,7 +1267,7 @@ success: err: assert(ret < 0); t->error = ret; - btr_remove_node(btrn); + btr_remove_node(&fn->btrn); } static void wmadec_open(struct filter_node *fn) diff --git a/write.c b/write.c index a8e2429c..de730008 100644 --- a/write.c +++ b/write.c @@ -143,7 +143,7 @@ pushdown: out: t->error = ret; if (ret < 0) - btr_remove_node(btrn); + btr_remove_node(&cwt->btrn); } static int loglevel; @@ -253,12 +253,12 @@ static int setup_and_schedule(void) struct writer *w = writers + wn->writer_num; w->close(wn); - btr_free_node(wn->btrn); + btr_remove_node(&wn->btrn); w->free_config(wn->conf); free(wn->conf); } free(wns); - btr_free_node(cwt->btrn); + btr_remove_node(&cwt->btrn); return ret; } -- 2.30.2