From ebffa40aa7832459f63801e3a91cc741e6b339a8 Mon Sep 17 00:00:00 2001 From: Etienne Dechamps Date: Sat, 20 Jun 2015 11:41:20 +0100 Subject: [PATCH] Protect against callbacks removing items from the io tree. The definition of the splay_each() macro is somewhat complicated for syntactic reasons. Here's what it does in a more readable way: for (splay_node_t* node = tree->head; node;) { type* item = node->data; splay_node_t* next = node->next; // RUN USER BLOCK with (item) node = next; } list_each() works in the same way. Since node->next is saved before the user block runs, this construct supports removing the current item from within the user block. However, what it does *not* support is removing *other items* from within the user block, especially the next item. Indeed, that will invalide the next pointer in the above loop and therefore result in an invalid pointer dereference. Unfortunately, there is at least one code path where that unsupported operation happens. It is located in ack_h(), where the authentication protocol code detects a double connection (i.e. being connected to another node twice). Running in the context of a socket read event, this code will happily terminate the *other* metaconnection, resulting in its socket being removed from the io tree. If, by misfortune, this other metaconnection happened to have the next socket FD number (which is quite possible due to FD reuse - albeit unlikely), and was part of the io tree (which is quite likely because if that connection is stuck, it will most likely have pending writes) then this will result in the next pending io item being destroyed. Invalid pointer dereference ensues. I did a quick audit of other uses of splay_each() and list_each() and I believe this is the only scenario in which this "next pointer invalidation" problem can occur in practice. While this bug has been there since at least 6bc5d626a8726fc23365ee705761a3c666a08ad4 (November 2012), if not sooner, it happens quite rarely due to the very specific set of conditions required to trigger it. Nevertheless, it does manage to crash my central production nodes every other week or so. --- src/event.c | 10 ++++++++++ src/list.h | 6 ++++++ src/splay_tree.h | 6 ++++++ 3 files changed, 22 insertions(+) diff --git a/src/event.c b/src/event.c index 60d357d4..59b96e37 100644 --- a/src/event.c +++ b/src/event.c @@ -285,6 +285,16 @@ bool event_loop(void) { io->cb(io->data, IO_WRITE); else if(FD_ISSET(io->fd, &readable)) io->cb(io->data, IO_READ); + else + continue; + + /* + There are scenarios in which the callback will remove another io_t from the tree + (e.g. closing a double connection). Since splay_each does not support that, we + need to exit the loop now. That's okay, since any remaining events will get picked + up by the next select() call. + */ + break; } } #else diff --git a/src/list.h b/src/list.h index 0437bd92..b0e7a845 100644 --- a/src/list.h +++ b/src/list.h @@ -79,6 +79,12 @@ extern void list_delete_list(list_t *); extern void list_foreach(list_t *, list_action_t); extern void list_foreach_node(list_t *, list_action_node_t); +/* + Iterates over a list. + + CAUTION: while this construct supports deleting the current item, + it does *not* support deleting *other* nodes while iterating on the list. + */ #define list_each(type, item, list) (type *item = (type *)1; item; item = NULL) for(list_node_t *node = (list)->head, *next; item = node ? node->data : NULL, next = node ? node->next : NULL, node; node = next) #endif /* __TINC_LIST_H__ */ diff --git a/src/splay_tree.h b/src/splay_tree.h index 58488707..3ddf2172 100644 --- a/src/splay_tree.h +++ b/src/splay_tree.h @@ -106,6 +106,12 @@ extern splay_node_t *splay_search_closest_greater_node(splay_tree_t *, const voi extern void splay_foreach(const splay_tree_t *, splay_action_t); extern void splay_foreach_node(const splay_tree_t *, splay_action_t); +/* + Iterates over a tree. + + CAUTION: while this construct supports deleting the current item, + it does *not* support deleting *other* nodes while iterating on the tree. + */ #define splay_each(type, item, tree) (type *item = (type *)1; item; item = NULL) for(splay_node_t *node = (tree)->head, *next; item = node ? node->data : NULL, next = node ? node->next : NULL, node; node = next) #endif -- 2.20.1