Commit graph

8 commits

Author SHA1 Message Date
Etienne Dechamps
ebffa40aa7 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 6bc5d626a8 (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.
2015-06-20 14:09:00 +01:00
Guus Sliepen
70a1a5594a Update copyright notices. 2012-10-14 17:42:49 +02:00
Guus Sliepen
d917c8cb6b Fix whitespace. 2012-10-10 17:17:49 +02:00
Guus Sliepen
f62b4a9134 Fix deleting connections from the connection list. 2012-10-09 13:23:12 +02:00
Guus Sliepen
0b8b23e0dd C99 extravaganza. 2012-10-08 00:35:38 +02:00
Guus Sliepen
80ca91769d Fix nodes joining the VPN after tincctl top started. 2011-06-02 21:14:50 +02:00
Guus Sliepen
4574b04f79 Allow inserting items in the middle of a list. 2011-05-15 16:29:54 +02:00
Guus Sliepen
35b1c25093 Move source from lib/ to src/.
The utility functions in the lib/ directory do not really form a library.
Also, now that we build two binaries, tincctl does not need everything that was
in libvpn.a, so it is wasteful to link to it.
2009-12-31 13:19:13 +01:00
Renamed from lib/list.h (Browse further)