View Issue Details

IDProjectCategoryView StatusLast Update
0004928unrealircdpublic2017-04-12 09:50
ReporterGottem Assigned Tosyzop  
Status closedResolutionno change required 
Product Version4.0.11 
Summary0004928: IsServer() call doesn't check for "me"
DescriptionOver the course of writing my modules I've encountered some peculiar crashes. I've ported the module m_chansno which posts "snomasks" to channels, as the server itself (so you see like "<> some msg"). It originally used do_cmd() to pass the message to all servers including itself (I figured out a better way to do this now, but the bug remains).

Apparently do_cmd() makes the IRCd run hooks and overrides, at which point things get funky if you also have modules that use channel-related hooks (e.g. the built-in floodprot, my m_textshun). It's probably easiest to see the bug if I write out a scenario, refer to the "steps to reproduce" for that.
Steps To Reproduce* Load m_chansno and add the config block. Something like this will do:
chansno {
    msgtype privmsg;

    channel "#somechan" {
        mode-changes; topics; joins; parts; kicks; nickchanges; connects; disconnects; server-connects; squits; unknown-users; channel-creations; channel-destructions; oper-ups; spamfilter-hits;

* Rehash, connect if needed and join #somechan
* Now enable floodprot on #somechan; any flag combo will do, I used +f [5t]:1
* OR load m_textshun and add a "T:Line" (floodprot is more familiar probably though =])
Both of these modules will run similar checks and the outcome is the same.

* Now do something to fire a chansnomask, like change your nick or join/part channels.
* floodprot will call is_skochanop() (which checks IsServer(cptr) first) to see if the user is exempt from +f. m_textshun will simply check IsServer(sptr).
* floodprot/is_skochanop() will then proceed to the membership loop involving cptr->user->channel. However, cptr->user is NULL for servers so ->channel results in a segfault.
* m_textshun will attempt to find a matching T:Line, for which it uses sptr->user, which is also NULL so ->username results in a segfault too.
Additional InformationI fixed m_textshun by doing !IsServer(sptr) && !IsMe(sptr), but this seems redundant. It's pretty much known that "me" is a server so IsServer(&me) should return a true value too.
TagsNo tags attached.
3rd party modulesAll of my own, but sorta N/A in this case



2017-04-12 09:48

administrator   ~0019744

I'm not comfortable changing IsServer due to the unknown impact. I would have to audit 160+ IsServer() calls just for this.

(Comments below are in general - I have not looked at your modules)

As for your modules, if you need to check some user rights then I suggest you use IsPerson() to validate it's a client first. It's a common module coding error not to do that.. blindly dereferencing fields on a client such as sptr->user will cause crashes.
So: use IsPerson() at proper places if you are calling user checks, that way you won't need an !IsServer() && !IsMe()

Similarly, if you are calling UnrealIRCd functions that otherwise would never be called directly, it's your own risk I'm afraid.
In general if you for example need to send a message to a channel you should do so yourself and not cut corners, see next.
More precisely, some module coders are lazy and call m_ function with &me. As you will see this is never done in the UnrealIRCd source (except for the m_tkl compatability layer) and you will likely crash while doing so. In fact even if you don't crash I would discourage it as there may be some future change that will make it crash.

As for do_cmd, that is the recommended way to re-fire a command from a real user. It will process the command as if it was typed by the user, including running overrides indeed.

Issue History

Date Modified Username Field Change
2017-04-05 17:19 Gottem New Issue
2017-04-12 09:48 syzop Note Added: 0019744
2017-04-12 09:50 syzop Assigned To => syzop
2017-04-12 09:50 syzop Status new => closed
2017-04-12 09:50 syzop Resolution open => no change required