anope/anope — pull request #15 by mokkori (2013-07-08 04:48:23+00:00)
Title: Some memory behaviour adjustments

HAI FRIENDLY ENGINEERS OF ANOPE I COME IN PEACE

I figured restarting your awesome software all the time to fix things was too silly so I sat down, did the math and ate all the vegetables. Eventually this resulted in all these little commits you see, teaching memory leaks or general bad memory access behaviour some real manners that I'd like to see merged.

I basically ran anope in valgrind (THE HORROR) until I was satisfied. Just starting up and closing down immediately leaked ~1,5mb (your mileage might vary, forgot if I removed custom modules when I did this the last time):

==15734== LEAK SUMMARY:

==15734== definitely lost: 1,564,301 bytes in 78,573 blocks

==15734== indirectly lost: 138,691 bytes in 3,215 blocks

Clearly, work needed to be done.

**Attention**: None of the commits should in any way collide with legacy module code, besides where explicitely noted.

# The "changes" #

Leaks (the big ones):

1. Every module unload/reload says bye bye to all custom language strings the module had loaded.

2. Calling moduleGetConfigDirective() decided to leak pretty much the whole config (minus the last line) at each call.

3. Every module message was lost on module unload/reload. Every module author obviously can fix this by properly unloading messages that he creates, however nobody does. Anope tutorials and example modules do not suggest such a requirement.

4. Every module command was lost on module unload/reload. **Warning:** this change has some ramifications that *will* cause problems for legacy modules if they followed anope behaviour. Properly unloading a module command also means free'ing its help_param1 char pointer. Sometimes however, this was simply made to point to s_NickServ/s_ChanServ. Free that and you're toast. *If* some module author used this feature wrongly in the past, this will cause a crash. Alternatively change this to never call free on help_param1. Still wobbly.

5. Every module event handler and event hook was leaked on module unload/reload.

6. There must be a pattern here. Every module config entry of type string was leaked on config reload

7. So basically, config reloads should have been forbidden. They leaked just about **everything**.

Leaks (the small and tiny ones):

1. Don't even ask how this happens, but module author and version strings could be leaked.

2. Module callbacks weren't properly cleaned up

3. Channel bans/invites and exceptions weren't properly cleaned up

4. Add a services ignore for someone who is already being ignored and leak a little

5. cs_appendtopic (official module), this one can actually become a problem if someone really, really, really tries long enough. Very very very long. Leaks the string to append on every call as well as the channel name.

6. hs_request leaks memory on config reload. But only if you decide to add this feature to it. So currently not. Still pointing it out, though.

7. This one was tricky to find, actually. Set PreNickServDB and don't set NSEmailReg and reload the config. Voila.

The misbehaving:

1. Invalid pointer read in slists (last read)

2. save_ns_dbase() leaks some uninitialised bytes into the db file

3. Actually a silly bug, but i still considered adding it here. Also tricky to find. Calling /os jupe on a server that is currently linked will forget to remove the server internally. Relink it and now anope believes the server to exist twice. Known side-effects are double globals to users on that server. Probably more. This can be repeated until it's no longer fun for anybody.

That's it as far as I'm concerned. I understand that these are sometimes serious changes from out of nowhere, so feel free to cut, slice, roast, dip and eat them dead or alive at will. Also look at them carefully.

Disclaimer: These changes have been in production on 2 networks (1 UnrealIRCd, 1 InspIRCd) for more or less 1 year now (the /os jupe fix for 5 months on 1 UnrealIRCd network) and I consider them reliable. Probably.

Quick c&p from bitbucket, because this place is better.