From 5877a32b3bce124aea40cd7de2ef0cabf125e754 Mon Sep 17 00:00:00 2001 From: Bram Matthys Date: Fri, 25 Jun 2004 23:50:08 +0000 Subject: [PATCH] - Fixed "quickly-rehashing + autoconnect linkblocks = crash"-bug. This involved fixing multiple reference count bugs, one related to sptr->serv->conf, and another one related to sptr->serv->class. Both caused problems when someone did a /rehash when a server was in the process of connecting (so it might also happen when connfreq was hit and you did a /rehash). Original bug was reported by sh0 (#0001872). --- Changes | 5 ++++ include/config.h | 6 +++- include/h.h | 2 ++ include/struct.h | 3 ++ src/modules/m_server.c | 10 +++++-- src/modules/m_stats.c | 12 ++++++-- src/s_bsd.c | 3 ++ src/s_conf.c | 64 ++++++++++++++++++++++++++++++------------ src/s_misc.c | 22 +++++++++++---- 9 files changed, 98 insertions(+), 29 deletions(-) diff --git a/Changes b/Changes index a90499f08..a7e5b0655 100644 --- a/Changes +++ b/Changes @@ -3313,3 +3313,8 @@ This is the 3.2 fixes branch. the correct path (#0001898) reported by AngryWolf - Updated HOOKTYPE_TKL_ADD/HOOKTYPE_TKL_DEL to cptr, sptr, tk, parc, parv, else it was impossible to tell *who* removed a *line. Again, parc/parv are 0/NULL for expires. +- Fixed "quickly-rehashing + autoconnect linkblocks = crash"-bug. This involved fixing + multiple reference count bugs, one related to sptr->serv->conf, and another one related + to sptr->serv->class. Both caused problems when someone did a /rehash when a server + was in the process of connecting (so it might also happen when connfreq was hit and you + did a /rehash). Original bug was reported by sh0 (#0001872). diff --git a/include/config.h b/include/config.h index 39a58435c..a7eba3a33 100644 --- a/include/config.h +++ b/include/config.h @@ -241,7 +241,11 @@ * the maintainer. */ -/* #undef DEBUGMODE define DEBUGMODE to enable debugging mode.*/ +/* DEBUGMODE: This should only be used when tracing a problem. It creates + * an insane amount of log output which can be very useful for debugging. + * You should *NEVER* enable this setting on production servers. + */ +/* #undef DEBUGMODE */ /* * Full pathnames and defaults of irc system's support files. Please note that diff --git a/include/h.h b/include/h.h index 770281ff7..c5624d4c8 100644 --- a/include/h.h +++ b/include/h.h @@ -701,3 +701,5 @@ extern int register_user(aClient *cptr, aClient *sptr, char *nick, char *usernam extern int on_dccallow_list(aClient *to, aClient *from); extern int add_dccallow(aClient *sptr, aClient *optr); extern int del_dccallow(aClient *sptr, aClient *optr); +extern void delete_linkblock(ConfigItem_link *link_ptr); +extern void delete_classblock(ConfigItem_class *class_ptr); diff --git a/include/struct.h b/include/struct.h index 106fd6ea3..271297001 100644 --- a/include/struct.h +++ b/include/struct.h @@ -1060,6 +1060,9 @@ struct _configitem_class { ConfigFlag flag; char *name; int pingfreq, connfreq, maxclients, sendq, recvq, clients; + int xrefcount; /* EXTRA reference count, 'clients' also acts as a reference count but + * link blocks also refer to classes so a 2nd ref. count was needed. + */ }; struct _configflag_allow { diff --git a/src/modules/m_server.c b/src/modules/m_server.c index 0d02fffa1..f7071018b 100644 --- a/src/modules/m_server.c +++ b/src/modules/m_server.c @@ -593,6 +593,7 @@ int m_server_synch(aClient *cptr, long numeric, ConfigItem_link *aconf) aClient *acptr; int i; char buf[BUFSIZE]; + int incoming = IsUnknown(cptr) ? 1 : 0; ircd_log(LOG_SERVER, "SERVER %s", cptr->name); @@ -601,7 +602,7 @@ int m_server_synch(aClient *cptr, long numeric, ConfigItem_link *aconf) MyFree(cptr->passwd); cptr->passwd = NULL; } - if (IsUnknown(cptr)) + if (incoming) { /* If this is an incomming connection, then we have just received * their stuff and now send our stuff back. @@ -684,7 +685,12 @@ int m_server_synch(aClient *cptr, long numeric, ConfigItem_link *aconf) cptr->srvptr = &me; cptr->serv->numeric = numeric; cptr->serv->conf = aconf; - cptr->serv->conf->refcount++; + if (incoming) + { + cptr->serv->conf->refcount++; + Debug((DEBUG_ERROR, "reference count for %s (%s) is now %d", + cptr->name, cptr->serv->conf->servername, cptr->serv->conf->refcount)); + } cptr->serv->conf->class->clients++; cptr->class = cptr->serv->conf->class; add_server_to_table(cptr); diff --git a/src/modules/m_stats.c b/src/modules/m_stats.c index cbde91a03..c0a299bfb 100644 --- a/src/modules/m_stats.c +++ b/src/modules/m_stats.c @@ -502,7 +502,7 @@ int stats_links(aClient *sptr, char *para) ConfigItem_link *link_p; for (link_p = conf_link; link_p; link_p = (ConfigItem_link *) link_p->next) { - sendto_one(sptr, ":%s 213 %s C %s@%s * %s %i %s %s%s%s%s%s", + sendto_one(sptr, ":%s 213 %s C %s@%s * %s %i %s %s%s%s%s%s%s", me.name, sptr->name, IsOper(sptr) ? link_p->username : "*", IsOper(sptr) ? link_p->hostname : "*", link_p->servername, link_p->port, @@ -511,7 +511,11 @@ int stats_links(aClient *sptr, char *para) (link_p->options & CONNECT_SSL) ? "S" : "", (link_p->options & CONNECT_ZIP) ? "z" : "", (link_p->options & CONNECT_NODNSCACHE) ? "d" : "", - (link_p->options & CONNECT_NOHOSTCHECK) ? "h" : ""); + (link_p->options & CONNECT_NOHOSTCHECK) ? "h" : "", + (link_p->flag.temporary == 1) ? "T" : ""); +#ifdef DEBUGMODE + sendnotice(sptr, "%s has refcount %d", link_p->servername, link_p->refcount); +#endif if (link_p->hubmask) sendto_one(sptr, ":%s 244 %s H %s * %s", me.name, sptr->name, link_p->hubmask, @@ -1420,6 +1424,10 @@ int stats_class(aClient *sptr, char *para) sendto_one(sptr, rpl_str(RPL_STATSYLINE), me.name, sptr->name, classes->name, classes->pingfreq, classes->connfreq, classes->maxclients, classes->sendq, classes->recvq ? classes->recvq : CLIENT_FLOOD); +#ifdef DEBUGMODE + sendnotice(sptr, "class '%s' has clients=%d, xrefcount=%d", + classes->name, classes->clients, classes->xrefcount); +#endif } return 0; } diff --git a/src/s_bsd.c b/src/s_bsd.c index 1c7eff6bd..e626de052 100644 --- a/src/s_bsd.c +++ b/src/s_bsd.c @@ -2445,6 +2445,9 @@ int connect_server(ConfigItem_link *aconf, aClient *by, struct hostent *hp) */ (void)make_server(cptr); cptr->serv->conf = aconf; + cptr->serv->conf->refcount++; + Debug((DEBUG_ERROR, "reference count for %s (%s) is now %d", + cptr->name, cptr->serv->conf->servername, cptr->serv->conf->refcount)); if (by && IsPerson(by)) { (void)strlcpy(cptr->serv->by, by->name, sizeof cptr->serv->by); diff --git a/src/s_conf.c b/src/s_conf.c index e90f92f14..436f2421e 100644 --- a/src/s_conf.c +++ b/src/s_conf.c @@ -1702,6 +1702,21 @@ void config_rehash() DelListItem(oper_ptr, conf_oper); MyFree(oper_ptr); } + for (link_ptr = conf_link; link_ptr; link_ptr = (ConfigItem_link *) next) + { + next = (ListStruct *)link_ptr->next; + if (link_ptr->refcount == 0) + { + Debug((DEBUG_ERROR, "s_conf: deleting block %s (refcount 0)", link_ptr->servername)); + delete_linkblock(link_ptr); + } + else + { + Debug((DEBUG_ERROR, "s_conf: marking block %s (refcount %d) as temporary", + link_ptr->servername, link_ptr->refcount)); + link_ptr->flag.temporary = 1; + } + } for (class_ptr = conf_class; class_ptr; class_ptr = (ConfigItem_class *) next) { next = (ListStruct *)class_ptr->next; @@ -1709,11 +1724,9 @@ void config_rehash() continue; class_ptr->flag.temporary = 1; /* We'll wipe it out when it has no clients */ - if (!class_ptr->clients) + if (!class_ptr->clients && !class_ptr->xrefcount) { - ircfree(class_ptr->name); - DelListItem(class_ptr, conf_class); - MyFree(class_ptr); + delete_classblock(class_ptr); } } for (uline_ptr = conf_ulines; uline_ptr; uline_ptr = (ConfigItem_ulines *) next) @@ -1757,20 +1770,6 @@ void config_rehash() MyFree(ban_ptr); } } - for (link_ptr = conf_link; link_ptr; link_ptr = (ConfigItem_link *) next) - { - next = (ListStruct *)link_ptr->next; - if (link_ptr->refcount == 0) - { - link_cleanup(link_ptr); - DelListItem(link_ptr, conf_link); - MyFree(link_ptr); - } - else - { - link_ptr->flag.temporary = 1; - } - } for (listen_ptr = conf_listen; listen_ptr; listen_ptr = (ConfigItem_listen *)listen_ptr->next) { listen_ptr->flag.temporary = 1; @@ -5298,6 +5297,7 @@ int _conf_link(ConfigFile *conf, ConfigEntry *ce) cep->ce_vardata); link->class = default_class; } + link->class->xrefcount++; for (cep = ce->ce_entries; cep; cep = cep->ce_next) { if (!strcmp(cep->ce_varname, "options")) @@ -7692,6 +7692,34 @@ void link_cleanup(ConfigItem_link *link_ptr) link_ptr->recvauth = NULL; } +void delete_linkblock(ConfigItem_link *link_ptr) +{ + Debug((DEBUG_ERROR, "delete_linkblock: deleting %s, refcount=%d", + link_ptr->servername, link_ptr->refcount)); + if (link_ptr->class) + { + link_ptr->class->xrefcount--; + /* Perhaps the class is temporary too and we need to free it... */ + if (link_ptr->class->flag.temporary && + !link_ptr->class->clients && !link_ptr->class->xrefcount) + { + delete_classblock(link_ptr->class); + link_ptr->class = NULL; + } + } + link_cleanup(link_ptr); + DelListItem(link_ptr, conf_link); + MyFree(link_ptr); +} + +void delete_classblock(ConfigItem_class *class_ptr) +{ + Debug((DEBUG_ERROR, "delete_classblock: deleting %s, clients=%d, xrefcount=%d", + class_ptr->name, class_ptr->clients, class_ptr->xrefcount)); + ircfree(class_ptr->name); + DelListItem(class_ptr, conf_class); + MyFree(class_ptr); +} void listen_cleanup() { diff --git a/src/s_misc.c b/src/s_misc.c index 745ecf1a4..4ca6ba19b 100644 --- a/src/s_misc.c +++ b/src/s_misc.c @@ -457,21 +457,31 @@ int exit_client(aClient *cptr, aClient *sptr, aClient *from, char *comment) delfrom_fdlist(sptr->slot, &serv_fdlist); #endif if (sptr->class) + { sptr->class->clients--; + if ((sptr->class->flag.temporary) && !sptr->class->clients && !sptr->class->xrefcount) + { + delete_classblock(sptr->class); + sptr->class = NULL; + } + } if (IsClient(sptr)) IRCstats.me_clients--; - if (IsServer(sptr)) + if (sptr->serv && sptr->serv->conf) { - IRCstats.me_servers--; sptr->serv->conf->refcount--; + Debug((DEBUG_ERROR, "reference count for %s (%s) is now %d", + sptr->name, sptr->serv->conf->servername, sptr->serv->conf->refcount)); if (!sptr->serv->conf->refcount && sptr->serv->conf->flag.temporary) { - /* Due for deletion */ - DelListItem(sptr->serv->conf, conf_link); - link_cleanup(sptr->serv->conf); - MyFree(sptr->serv->conf); + Debug((DEBUG_ERROR, "deleting temporary block %s", sptr->serv->conf->servername)); + delete_linkblock(sptr->serv->conf); } + } + if (IsServer(sptr)) + { + IRCstats.me_servers--; ircd_log(LOG_SERVER, "SQUIT %s (%s)", sptr->name, comment); }