[IRCServices] Allocation error: ns unsuspend [retry]

[Chawmp]

Yeah... It seems like the kind of problem that could have been lying
undetected in a number of places for some time.

The first bug I posted about took a lot of trial and error to locate
exactly, but with the help of the memory management debugger (among other
things) Valgrind [ http://valgrind.kde.org/ ] I was able to find the source
of this problem much more quickly. If you're not using something like it
already, it might be of help. Of course, to find every bug of this kind you
would need to potentially cause every possible flow-of-control to happen. I
guess picking out appropriate sections to examine, as you were doing, would
make the process more feasible :)

Tom McIntyre
chawmp at cyberarmy.net

> -----Original Message-----
> From: ircservices-bounces at ircservices.za.net [mailto:ircservices-
> bounces at ircservices.za.net] On Behalf Of Andrew Church
> Sent: 26 July 2004 14:30
> To: ircservices at ircservices.za.net
> Subject: Re: [IRCServices] Allocation error: ns unsuspend [retry]
> 
>      I did see your original message--it's just that I've been going
> through the entire code to find and fix similar bugs as well.  I have a
> sneaking suspicion that the expire-on-get design plays nasty games with
> pointers in the user record too, but as it hasn't seemed to cause any
> problems so far, I'm saving that bit of contemplation for 5.1 (which,
> incidentally, will also have similar use-after-free checks in the memory
> checking code).
> 
>      At any rate, thanks for the report.
> 
>   --Andrew Church
>     achurch at achurch.org
>     http://achurch.org/
> 
> >Hi again,
> >
> >[ I sent this yesterday, but it seems it got put under another thread in
> the
> >archive, and didn't actually get sent out to the list, so I'll send it
> again
> >in case it gets missed :) ]
> >
> >I just noticed and patched another bug, quite similar to the last issue I
> >posted about ("Seg Fault/Bus Error on SQUIT":
> >http://www.ircservices.za.net/pipermail/ircservices/2004/003377.html ).
> >
> >Again, I noticed this one because free() is set up to write a pattern
> over
> >memory as it is released on our services host. It wouldn't occur in most
> >normal circumstances, but might do unpredictably, depending on a lot of
> >factors, so ought to be fixed.
> >
> >The problem occurs when unsuspending a nickname that is part of a group
> in
> >which no nickname has been used for longer than the NSExpire setting - or
> >something along those lines.
> >
> >In modules/nickserv/util.c, unsuspend_nick() does this:
> >
> >	ARRAY_FOREACH (i, ngi->nicks) {
> >		NickInfo *ni = get_nickinfo(ngi->nicks[i]);
> >		...
> >	}
> >
> >get_nickinfo() will free (NickGroupInfo *)ngi under certain conditions
> >(roughly as I described above), making the following attempts to
> dereference
> >ngi crash the program:
> >
> >		if (!ni) {
> >			module_log("unsuspend: unable to retrieve NickInfo
> >for %s"
> >				" (nick group %u)", ngi->nicks[i], ngi->id);
> >			continue;
> >		}
> >
> >ngi would also be used in subsequent loops, so just changing the log
> message
> >wouldn't be a solution.
> >
> >I didn't have a lot of time to investigate the best way to fix this, but
> >here is the patch I came up with. It seems to do the job, but I would be
> >grateful if anyone can advise something more suitable. (Lines beginning
> "+"
> >were added; noexpire is just set to 1 before the loop, and restored
> >afterwards, which stops the expiry check. Note that the NSSuspendGrace
> stuff
> >only happens a few lines after the call to get_nickinfo(), so for that
> small
> >time, nick groups can disappear.).
> >
> >  void unsuspend_nick(NickGroupInfo *ngi, int set_time)
> >  {
> >      time_t now = time(NULL);
> >+ int cache_noexpire = 0;
> >
> >      if (!ngi->suspendinfo) {
> >  	module_log("unsuspend: called on non-suspended nick group %u [%s]",
> >
> >...
> >
> >  		       " %u) to %ld", ngi->nicks[ngi->mainnick], ngi->id,
> >  		       (long)ngi->authset);
> >  	}
> >+ cache_noexpire = noexpire;
> >+ noexpire = 1;
> >  	ARRAY_FOREACH (i, ngi->nicks) {
> >  	    NickInfo *ni = get_nickinfo(ngi->nicks[i]);
> >  	    if (!ni) {
> >
> >...
> >
> >  	    }
> >  	}
> >      }
> >+ noexpire = cache_noexpire;
> >  }
> >
> >
> >/************************************************************************
> */
> >
> >--
> >Tom McIntyre
> >chawmp at cyberarmy.net
>