Subject: mrouted restart [SIGHUP] processing broken

From: "Danny J. Mitzel" <[email-masked]>
Date: Thu, 04 Apr 1996 11:29:15 -0800

I spent a bit of time last week tracking down a strange problem where
IGMP group state was getting blown away even though it was being
actively refreshed [and this leads to the group being immediately pruned
on leaf networks :-[  turns out on SIGHUP processing that the group
membership state is freed but timers are left running and pointing to
the old memory segments.  after restart new group memberships get deposited
into the memory areas and new timers allocated.  at a later time the old
timers expire and blow away state, then new reports re-establish
group state, then old timers expire, ...  I've attached an unofficial
patch below.

on a more general note; while looking through the code for possible
race conditions that might cause the above problem I noted that the
callout queue code recognizes that the daemon has multiple threads of
execution and employs some simple locking, but none of the group membership,
routing table, kernel table etc. have any locking.  I believe it can
be shown that there are a number of places where the routing table or
other data structure might be in the process of update [for example
after receiving a new route] during which the SIGALARM
``thread'' could fire and also attempt to update data structures
[such as timing out a route].  seems it might be safer [and quite easy]
to serialize all message triggered processing and periodic processing
via the select w/timeout, rather than the separate threads of control
w/select + SIGALARM.  just a suggestion.

cheers,
danny

*** 1.1	1996/03/25 21:00:44
--- vif.c	1996/04/04 19:22:21
***************
*** 408,413 ****
--- 408,418 ----
  	while (v->uv_groups != NULL) {
  	    a = v->uv_groups;
  	    v->uv_groups = a->al_next;
+ 	    /* delete active group timers */
+ 	    if (a->al_timerid)
+ 		DeleteTimer(a->al_timerid);
+ 	    if (a->al_query)
+ 		DeleteTimer(a->al_query);
  	    free((char *)a);
  	}
  	while (v->uv_neighbors != NULL) {

---

From: Bill Fenner <[email-masked]>
Date: Thu, 4 Apr 1996 12:11:29 PST

Danny,

  Thanks for the patch.  It might be safer to just free the whole callout
queue on restart (in fact, restart() should have called callout_init(),
then this would just be a memory leak...)

>seems it might be safer [and quite easy]
>via the select w/timeout

Already done in mrouted 4.0, for the exact reason that locking was getting
too difficult.

  Bill