From 3461b6ad5cb90f1448360d0eb04dd661ae477610 Mon Sep 17 00:00:00 2001 From: Jeremy Kerr Date: Fri, 17 May 2013 09:38:07 +0800 Subject: lib/waiter: Ensure waiters are consistent during waiter_poll We have a bug at the moment: if the waitset's->waiters array is updated duing waiter_poll() (eg, a client connection is closed, and the client's callback performs a waiter_remove()), then we may invoke callbacks for incorrect waiters. This change uses a consistent waiters array duing execution of waiter_poll, so that any pollfds returned from poll() will result in correct callback invocations. This assumes that a waiter will only ever remove *itself* from the waitset; otherwise, we may call a free()ed waiter. Signed-off-by: Jeremy Kerr --- lib/waiter/waiter.c | 62 +++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 19 deletions(-) diff --git a/lib/waiter/waiter.c b/lib/waiter/waiter.c index bb25784..78ba045 100644 --- a/lib/waiter/waiter.c +++ b/lib/waiter/waiter.c @@ -1,5 +1,6 @@ #include +#include #include #include @@ -18,8 +19,14 @@ struct waiter { struct waitset { struct waiter **waiters; int n_waiters; + bool waiters_changed; + + /* These are kept consistent over each call to waiter_poll, as + * set->waiters may be updated (by waiters' callbacks calling + * waiter_register or waiter_remove) during iteration. */ struct pollfd *pollfds; - int n_pollfds; + struct waiter **cur_waiters; + int cur_n_waiters; }; struct waitset *waitset_create(void *ctx) @@ -38,19 +45,22 @@ struct waiter *waiter_register(struct waitset *set, int fd, int events, { struct waiter **waiters, *waiter; + waiter = talloc(set->waiters, struct waiter); + if (!waiter) + return NULL; + waiters = talloc_realloc(set, set->waiters, struct waiter *, set->n_waiters + 1); - if (!waiters) + if (!waiters) { + talloc_free(waiter); return NULL; + } + set->waiters_changed = true; set->waiters = waiters; set->n_waiters++; - waiter = talloc(set->waiters, struct waiter); - if (!waiter) - return NULL; - set->waiters[set->n_waiters - 1] = waiter; waiter->set = set; @@ -79,6 +89,7 @@ void waiter_remove(struct waiter *waiter) set->waiters = talloc_realloc(set->waiters, set->waiters, struct waiter *, set->n_waiters); + set->waiters_changed = true; talloc_free(waiter); } @@ -87,29 +98,42 @@ int waiter_poll(struct waitset *set) { int i, rc; - if (set->n_waiters != set->n_pollfds) { - set->pollfds = talloc_realloc(set, set->pollfds, - struct pollfd, set->n_waiters); - set->n_pollfds = set->n_waiters; - } + /* If the waiters have been updated, we need to update our + * consistent copy */ + if (set->waiters_changed) { + + /* We need to reallocate if the count has changes */ + if (set->cur_n_waiters != set->n_waiters) { + set->cur_waiters = talloc_realloc(set, set->cur_waiters, + struct waiter *, set->n_waiters); + set->pollfds = talloc_realloc(set, set->pollfds, + struct pollfd, set->n_waiters); + set->cur_n_waiters = set->n_waiters; + } + + /* Populate cur_waiters and pollfds from ->waiters data */ + for (i = 0; i < set->n_waiters; i++) { + set->pollfds[i].fd = set->waiters[i]->fd; + set->pollfds[i].events = set->waiters[i]->events; + set->pollfds[i].revents = 0; + set->cur_waiters[i] = set->waiters[i]; + } - for (i = 0; i < set->n_waiters; i++) { - set->pollfds[i].fd = set->waiters[i]->fd; - set->pollfds[i].events = set->waiters[i]->events; - set->pollfds[i].revents = 0; + set->waiters_changed = false; } - rc = poll(set->pollfds, set->n_waiters, -1); + rc = poll(set->pollfds, set->cur_n_waiters, -1); if (rc <= 0) return rc; - for (i = 0; i < set->n_waiters; i++) { + for (i = 0; i < set->cur_n_waiters; i++) { if (set->pollfds[i].revents) { - rc = set->waiters[i]->callback(set->waiters[i]->arg); + rc = set->cur_waiters[i]->callback( + set->cur_waiters[i]->arg); if (rc) - waiter_remove(set->waiters[i]); + waiter_remove(set->cur_waiters[i]); } } -- cgit v1.2.1