[Click] [PATCH] usermode Router::run_selects() not robust under handler deletion

[Peter Swain]

I've been using Click for a connection-oriented project, where the event
handlers called from run_selects() themselves add or remove select handlers.
This was working great with click-1.2.4, but broke on upgrade to the CVS tree.
I took some time to find the problem: the Click codebase is such good quality
that I sought the problem exclusively in my own code first :)

Two potential problems here, in the HAVE_POLL_H case, related to add_select()
and remove_select():

When a handler is removed, _pollfds vector is compacted by...
        *p = _pollfds.back();
	_pollfds.pop_back();
but the associated _read_poll_elements[] & _write_poll_elements[],
which should be 1:1 with _pollfds[], are not similarly compacted.

When run_selects() calls a handler which registers new file descriptors,
it may cause _pollfds[] to be realloc()ed, invalidating the pointer p
which walks the vector.

The attached patch fixes both of these.

I dropped the optimization in the 3rd fragment, in which self-deletion of
handlers causes a re-scan of the new occupant of the slot. Nice trick,
but it was also vulnerable to vector realloc problems if one of the handlers
called add_select().
The next poll() will catch the moved handler anyway.

If this behavior is required, perhaps remove_select() should simply mark slots
empty, with events==0 and fd==-1, increment a reshuffle_required flag,
and have run_selects compact the vectors if reshuffle_required, after running
all the handlers.

Obviously noone else is removing handlers before router teardown, or adding them 
from within run_selects(). I seem to be bending the usage model somewhat!

^..^
(oo)
-------------- next part --------------
diff -ubwr click/lib/router.cc poll-fix/lib/router.cc
--- click/lib/router.cc	Fri Aug 29 18:02:47 2003
+++ poll-fix/lib/router.cc	Fri Oct 31 10:17:48 2003
@@ -1444,6 +1444,11 @@
       if (!p->events) {
 	*p = _pollfds.back();
 	_pollfds.pop_back();
+	// keep fds and elements in sync...
+	_write_poll_elements[pi]  = _write_poll_elements.back();
+	_write_poll_elements.pop_back();
+	_read_poll_elements[pi]  = _read_poll_elements.back();
+	_read_poll_elements.pop_back();
 #if !HAVE_POLL_H
 	if (!_pollfds.size())
 	  _max_select_fd = -1;
@@ -1502,7 +1507,8 @@
   if (n < 0 && errno != EINTR)
     perror("poll");
   else if (n > 0) {
-    for (struct pollfd *p = _pollfds.begin(); p < _pollfds.end(); p++)
+    for (int pi = 0; pi < _pollfds.size(); pi++) {
+      struct pollfd *p = &_pollfds[pi];
       if (p->revents & (POLLIN | POLLOUT)) {
 	int pi = p - _pollfds.begin();
 
@@ -1518,9 +1524,7 @@
 	  _elements[read_elt]->selected(fd);
 	if (write_elt >= 0 && write_elt != read_elt)
 	  _elements[write_elt]->selected(fd);
-
-	if (p < _pollfds.end() && fd != p->fd)
-	  p--;
+      }
       }
   }