View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0004928 | unreal | ircd | public | 2017-04-05 17:19 | 2017-04-12 09:50 |
| Reporter | Gottem | Assigned To | syzop | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | closed | Resolution | no change required | ||
| Platform | x86_64 | OS | Debian | OS Version | 7 (wheezy) |
| Product Version | 4.0.11 | ||||
| Summary | 0004928: IsServer() call doesn't check for "me" | ||||
| Description | Over 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 "<hub.my.net> 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 Information | I 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. | ||||
| Tags | No tags attached. | ||||
| 3rd party modules | All of my own, but sorta N/A in this case | ||||
|
|
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. |
