From e16c99cd2a923fe02015958785b01ac56a271b54 Mon Sep 17 00:00:00 2001 From: "Todd C. Miller" Date: Sat, 26 Oct 2013 06:55:23 -0600 Subject: [PATCH] Add a list of active events in the base that the back end sets when it calls poll or select. This allows the front end to iterate over the events instead of having that code in both back ends. It will also simplify support for timeout events. Also make sure we can't touch freed memory if a callback frees its own event. --- common/event.c | 84 ++++++++++++++++++++++++++--- common/event_poll.c | 73 ++++++++----------------- common/event_select.c | 121 ++++++++++++++++-------------------------- include/sudo_event.h | 26 +++++---- 4 files changed, 160 insertions(+), 144 deletions(-) diff --git a/common/event.c b/common/event.c index e32cc2974..32c162e5b 100644 --- a/common/event.c +++ b/common/event.c @@ -102,6 +102,10 @@ void sudo_ev_free(struct sudo_event *ev) { debug_decl(sudo_ev_free, SUDO_DEBUG_EVENT) + + /* Make sure ev is not in use before freeing it. */ + if (ev->base != NULL) + (void)sudo_ev_del(NULL, ev); free(ev); debug_return; } @@ -123,7 +127,7 @@ sudo_ev_add(struct sudo_event_base *base, struct sudo_event *ev, bool tohead) } } /* Clear pending delete so adding from callback works properly. */ - CLR(ev->events, SUDO_EV_DELETE); + CLR(ev->flags, SUDO_EV_DELETE); debug_return_int(0); } @@ -155,20 +159,30 @@ sudo_ev_del(struct sudo_event_base *base, struct sudo_event *ev) if (sudo_ev_del_impl(base, ev) != 0) debug_return_int(-1); - /* Unlink from list. */ + /* Unlink from event list. */ TAILQ_REMOVE(&base->events, ev, entries); - /* If we removed the pending event, replace it with the next one. */ - if (base->pending == ev) - base->pending = TAILQ_NEXT(ev, entries); + /* Unlink from active list and update base pointers as needed. */ + if (ISSET(ev->flags, SUDO_EV_ACTIVE)) { + TAILQ_REMOVE(&base->active, ev, active_entries); + if (ev == base->pending) + base->pending = TAILQ_NEXT(ev, active_entries); + if (ev == base->cur) + base->cur = NULL; + } /* Mark event unused. */ - ev->pfd_idx = -1; ev->base = NULL; + ev->flags = 0; + ev->pfd_idx = -1; debug_return_int(0); } +/* + * Run main event loop. + * Returns 0 on success, 1 if no events registered and -1 on error + */ int sudo_ev_loop(struct sudo_event_base *base, int flags) { @@ -184,8 +198,62 @@ sudo_ev_loop(struct sudo_event_base *base, int flags) flags |= SUDO_EVLOOP_ONCE; base->flags = 0; - /* Most work is done by the backend. */ - rc = sudo_ev_loop_impl(base, flags); + for (;;) { +rescan: + /* Make sure we have some events. */ + if (TAILQ_EMPTY(&base->events)) { + rc = 1; + break; + } + + /* Call backend to setup the active queue. */ + TAILQ_INIT(&base->active); + rc = sudo_ev_loop_impl(base, flags); + if (rc == -1) { + if (errno == EINTR || errno == ENOMEM) + continue; + break; + } + + /* + * Service each event in the active queue. + * We store the current event pointer in the base so that + * it can be cleared by sudo_ev_del(). This prevents a use + * after free if the callback frees its own event. + */ + TAILQ_FOREACH_SAFE(base->cur, &base->active, active_entries, base->pending) { + if (!ISSET(base->cur->events, SUDO_EV_PERSIST)) + SET(base->cur->flags, SUDO_EV_DELETE); + base->cur->callback(base->cur->fd, base->cur->revents, + base->cur->closure); + if (base->cur != NULL) { + CLR(base->cur->flags, SUDO_EV_ACTIVE); + if (ISSET(base->cur->flags, SUDO_EV_DELETE)) + sudo_ev_del(base, base->cur); + } + if (ISSET(base->flags, SUDO_EVBASE_LOOPBREAK)) { + /* stop processing events immediately */ + base->flags |= SUDO_EVBASE_GOT_BREAK; + base->pending = NULL; + goto done; + } + if (ISSET(base->flags, SUDO_EVBASE_LOOPCONT)) { + /* rescan events and start polling again */ + CLR(base->flags, SUDO_EVBASE_LOOPCONT); + base->pending = NULL; + goto rescan; + } + } + base->pending = NULL; + if (ISSET(base->flags, SUDO_EVBASE_LOOPEXIT)) { + /* exit loop after once through */ + base->flags |= SUDO_EVBASE_GOT_EXIT; + goto done; + } + if (flags & (SUDO_EVLOOP_ONCE | SUDO_EVLOOP_NONBLOCK)) + break; + } +done: base->flags &= SUDO_EVBASE_GOT_MASK; debug_return_int(rc); } diff --git a/common/event_poll.c b/common/event_poll.c index 4f153b5a0..d6bd7aa07 100644 --- a/common/event_poll.c +++ b/common/event_poll.c @@ -138,58 +138,31 @@ sudo_ev_loop_impl(struct sudo_event_base *base, int flags) int nready; debug_decl(sudo_ev_loop_impl, SUDO_DEBUG_EVENT) -rescan: - while (base->pfd_high != -1) { - nready = poll(base->pfds, base->pfd_high + 1, timeout); - sudo_debug_printf(SUDO_DEBUG_INFO, "%s: %d fds ready", - __func__, nready); - switch (nready) { - case -1: - if (errno == EINTR || errno == ENOMEM) - continue; - debug_return_int(-1); - case 0: - /* timeout or no events */ - break; - default: - /* Service each event that fired. */ - TAILQ_FOREACH_SAFE(ev, &base->events, entries, base->pending) { - if (ev->pfd_idx != -1 && base->pfds[ev->pfd_idx].revents) { - int what = 0; - if (base->pfds[ev->pfd_idx].revents & (POLLIN|POLLHUP|POLLNVAL|POLLERR)) - what |= (ev->events & SUDO_EV_READ); - if (base->pfds[ev->pfd_idx].revents & (POLLOUT|POLLHUP|POLLNVAL|POLLERR)) - what |= (ev->events & SUDO_EV_WRITE); - if (!ISSET(ev->events, SUDO_EV_PERSIST)) - SET(ev->events, SUDO_EV_DELETE); - ev->callback(ev->fd, what, ev->closure); - if (ISSET(ev->events, SUDO_EV_DELETE)) - sudo_ev_del(base, ev); - if (ISSET(base->flags, SUDO_EVBASE_LOOPBREAK)) { - /* stop processing events immediately */ - base->flags |= SUDO_EVBASE_GOT_BREAK; - base->pending = NULL; - goto done; - } - if (ISSET(base->flags, SUDO_EVBASE_LOOPCONT)) { - /* rescan events and start polling again */ - CLR(base->flags, SUDO_EVBASE_LOOPCONT); - base->pending = NULL; - goto rescan; - } - } + nready = poll(base->pfds, base->pfd_high + 1, timeout); + sudo_debug_printf(SUDO_DEBUG_INFO, "%s: %d fds ready", __func__, nready); + switch (nready) { + case -1: + /* error or interrupted by signal */ + debug_return_int(-1); + case 0: + /* timeout or no events */ + break; + default: + /* Activate each event that fired. */ + TAILQ_FOREACH(ev, &base->events, entries) { + if (ev->pfd_idx != -1 && base->pfds[ev->pfd_idx].revents) { + int what = 0; + if (base->pfds[ev->pfd_idx].revents & (POLLIN|POLLHUP|POLLNVAL|POLLERR)) + what |= (ev->events & SUDO_EV_READ); + if (base->pfds[ev->pfd_idx].revents & (POLLOUT|POLLHUP|POLLNVAL|POLLERR)) + what |= (ev->events & SUDO_EV_WRITE); + /* Make event active. */ + ev->revents = what; + SET(ev->flags, SUDO_EV_ACTIVE); + TAILQ_INSERT_TAIL(&base->active, ev, active_entries); } - base->pending = NULL; - if (ISSET(base->flags, SUDO_EVBASE_LOOPEXIT)) { - /* exit loop after once through */ - base->flags |= SUDO_EVBASE_GOT_EXIT; - goto done; - } - break; } - if (flags & (SUDO_EVLOOP_ONCE | SUDO_EVLOOP_NONBLOCK)) - break; + break; } -done: debug_return_int(0); } diff --git a/common/event_select.c b/common/event_select.c index dfb8b70a6..1410f3488 100644 --- a/common/event_select.c +++ b/common/event_select.c @@ -62,7 +62,6 @@ sudo_ev_base_alloc_impl(struct sudo_event_base *base) debug_decl(sudo_ev_base_alloc_impl, SUDO_DEBUG_EVENT) base->maxfd = NFDBITS - 1; - base->nevents = 0; base->readfds = ecalloc(1, sizeof(fd_mask)); base->writefds = ecalloc(1, sizeof(fd_mask)); @@ -92,7 +91,6 @@ sudo_ev_add_impl(struct sudo_event_base *base, struct sudo_event *ev) base->writefds = ecalloc(n, sizeof(fd_mask)); base->maxfd = (n * NFDBITS) - 1; } - base->nevents++; debug_return_int(0); } @@ -105,7 +103,6 @@ sudo_ev_del_impl(struct sudo_event_base *base, struct sudo_event *ev) /* Remove from readfds and writefds and adjust event count. */ FD_CLR(ev->fd, base->readfds); FD_CLR(ev->fd, base->writefds); - base->nevents--; debug_return_int(0); } @@ -115,7 +112,7 @@ sudo_ev_loop_impl(struct sudo_event_base *base, int flags) { struct timeval tv, *timeout; struct sudo_event *ev; - int nready; + int nready, highfd = 0; debug_decl(sudo_ev_loop, SUDO_DEBUG_EVENT) if (ISSET(flags, SUDO_EVLOOP_NONBLOCK)) { @@ -126,80 +123,52 @@ sudo_ev_loop_impl(struct sudo_event_base *base, int flags) timeout = NULL; } -rescan: - while (base->nevents != 0) { - int highfd = 0; - /* For select we need to redo readfds and writefds each time. */ - memset(base->readfds, 0, howmany(base->maxfd + 1, NFDBITS) * sizeof(fd_mask)); - memset(base->writefds, 0, howmany(base->maxfd + 1, NFDBITS) * sizeof(fd_mask)); - TAILQ_FOREACH(ev, &base->events, entries) { - if (ISSET(ev->events, SUDO_EV_READ)) { - sudo_debug_printf(SUDO_DEBUG_DEBUG, "%s: added fd %d to readfs", - __func__, ev->fd); - FD_SET(ev->fd, base->readfds); - if (ev->fd > highfd) - highfd = ev->fd; - } - if (ISSET(ev->events, SUDO_EV_WRITE)) { - sudo_debug_printf(SUDO_DEBUG_DEBUG, "%s: added fd %d to writefds", - __func__, ev->fd); - FD_SET(ev->fd, base->writefds); - if (ev->fd > highfd) - highfd = ev->fd; - } + /* For select we need to redo readfds and writefds each time. */ + memset(base->readfds, 0, howmany(base->maxfd + 1, NFDBITS) * sizeof(fd_mask)); + memset(base->writefds, 0, howmany(base->maxfd + 1, NFDBITS) * sizeof(fd_mask)); + TAILQ_FOREACH(ev, &base->events, entries) { + if (ISSET(ev->events, SUDO_EV_READ)) { + sudo_debug_printf(SUDO_DEBUG_DEBUG, "%s: added fd %d to readfs", + __func__, ev->fd); + FD_SET(ev->fd, base->readfds); + if (ev->fd > highfd) + highfd = ev->fd; } - sudo_debug_printf(SUDO_DEBUG_DEBUG, "%s: select high fd %d", - __func__, highfd); - nready = select(highfd + 1, base->readfds, base->writefds, NULL, timeout); - sudo_debug_printf(SUDO_DEBUG_INFO, "%s: %d fds ready", - __func__, nready); - switch (nready) { - case -1: - if (errno == EINTR || errno == ENOMEM) - continue; - debug_return_int(-1); - case 0: - /* timeout or no events */ - break; - default: - /* Service each event that fired. */ - TAILQ_FOREACH_SAFE(ev, &base->events, entries, base->pending) { - int what = 0; - if (FD_ISSET(ev->fd, base->readfds)) - what |= (ev->events & SUDO_EV_READ); - if (FD_ISSET(ev->fd, base->writefds)) - what |= (ev->events & SUDO_EV_WRITE); - if (what != 0) { - if (!ISSET(ev->events, SUDO_EV_PERSIST)) - SET(ev->events, SUDO_EV_DELETE); - ev->callback(ev->fd, what, ev->closure); - if (ISSET(ev->events, SUDO_EV_DELETE)) - sudo_ev_del(base, ev); - if (ISSET(base->flags, SUDO_EVBASE_LOOPBREAK)) { - /* stop processing events immediately */ - base->flags |= SUDO_EVBASE_GOT_BREAK; - base->pending = NULL; - goto done; - } - if (ISSET(base->flags, SUDO_EVBASE_LOOPCONT)) { - /* rescan events and start polling again */ - CLR(base->flags, SUDO_EVBASE_LOOPCONT); - base->pending = NULL; - goto rescan; - } - } - } - base->pending = NULL; - if (ISSET(base->flags, SUDO_EVBASE_LOOPEXIT)) { - /* exit loop after once through */ - base->flags |= SUDO_EVBASE_GOT_EXIT; - goto done; - } - break; + if (ISSET(ev->events, SUDO_EV_WRITE)) { + sudo_debug_printf(SUDO_DEBUG_DEBUG, "%s: added fd %d to writefds", + __func__, ev->fd); + FD_SET(ev->fd, base->writefds); + if (ev->fd > highfd) + highfd = ev->fd; } - if (flags & (SUDO_EVLOOP_ONCE | SUDO_EVLOOP_NONBLOCK)) - break; } -done: + sudo_debug_printf(SUDO_DEBUG_DEBUG, "%s: select high fd %d", + __func__, highfd); + nready = select(highfd + 1, base->readfds, base->writefds, NULL, timeout); + sudo_debug_printf(SUDO_DEBUG_INFO, "%s: %d fds ready", __func__, nready); + switch (nready) { + case -1: + /* error or interrupted by signal */ + debug_return_int(-1); + case 0: + /* timeout or no events */ + break; + default: + /* Activate each event that fired. */ + TAILQ_FOREACH(ev, &base->events, entries) { + int what = 0; + if (FD_ISSET(ev->fd, base->readfds)) + what |= (ev->events & SUDO_EV_READ); + if (FD_ISSET(ev->fd, base->writefds)) + what |= (ev->events & SUDO_EV_WRITE); + if (what != 0) { + /* Make event active. */ + ev->revents = what; + SET(ev->flags, SUDO_EV_ACTIVE); + TAILQ_INSERT_TAIL(&base->active, ev, active_entries); + } + } + break; + } debug_return_int(0); } diff --git a/include/sudo_event.h b/include/sudo_event.h index bc1150b85..84490643c 100644 --- a/include/sudo_event.h +++ b/include/sudo_event.h @@ -20,10 +20,13 @@ #include "queue.h" /* Event types */ -#define SUDO_EV_READ 0x01 /* fire when readable */ -#define SUDO_EV_WRITE 0x02 /* fire when writable */ -#define SUDO_EV_PERSIST 0x04 /* persist until deleted */ -#define SUDO_EV_DELETE 0x08 /* deletion pending */ +#define SUDO_EV_READ 0x01 /* fire when readable */ +#define SUDO_EV_WRITE 0x02 /* fire when writable */ +#define SUDO_EV_PERSIST 0x04 /* persist until deleted */ + +/* Event flags (internal) */ +#define SUDO_EV_ACTIVE 0x01 /* event is on the active queue */ +#define SUDO_EV_DELETE 0x02 /* deletion pending */ /* Event loop flags */ #define SUDO_EVLOOP_ONCE 0x01 /* Only run once through the loop */ @@ -42,10 +45,13 @@ typedef void (*sudo_ev_callback_t)(int fd, int what, void *closure); /* Member of struct sudo_event_base. */ struct sudo_event { TAILQ_ENTRY(sudo_event) entries; + TAILQ_ENTRY(sudo_event) active_entries; struct sudo_event_base *base; /* base this event belongs to */ int fd; /* fd we are interested in */ - short events; /* SUDO_EV_* flags */ - short pfd_idx; /* index into pfds array */ + short events; /* SUDO_EV_* flags (in) */ + short revents; /* SUDO_EV_* flags (out) */ + short flags; /* internal event flags */ + short pfd_idx; /* index into pfds array (XXX) */ sudo_ev_callback_t callback;/* user-provided callback */ void *closure; /* user-provided data pointer */ }; @@ -53,9 +59,10 @@ struct sudo_event { TAILQ_HEAD(sudo_event_list, sudo_event); struct sudo_event_base { - struct sudo_event_list events; /* tail queue of events */ - /* XXX - also list of active events and timed events */ - struct sudo_event *pending; /* next event to be run in the event loop XXX */ + struct sudo_event_list events; /* tail queue of all events */ + struct sudo_event_list active; /* tail queue of active events */ + struct sudo_event *cur; /* current active event being serviced */ + struct sudo_event *pending; /* next active event to be serviced */ #ifdef HAVE_POLL struct pollfd *pfds; /* array of struct pollfd */ int pfd_max; /* size of the pfds array */ @@ -65,7 +72,6 @@ struct sudo_event_base { fd_set *readfds; /* read I/O descriptor set */ fd_set *writefds; /* write I/O descriptor set */ int maxfd; /* max fd we can store in readfds/writefds */ - int nevents; /* number of events in the list */ #endif /* HAVE_POLL */ unsigned int flags; /* SUDO_EVBASE_* */ };