View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0005135 | unreal | module api | public | 2018-08-24 22:30 | 2018-09-06 21:11 |
| Reporter | Gottem | Assigned To | syzop | ||
| Priority | normal | Severity | tweak | Reproducibility | always |
| Status | closed | Resolution | no change required | ||
| Product Version | 4.0.18 | ||||
| Summary | 0005135: do_cmd() for TOPIC doesn't account for IsMe() | ||||
| Description | I'm trying to write a module that makes the server itself set a channel topic. But m_topic only accounts for IsULine() and IsServer(), neither of which match &me. The call is as follows: char *p[6] = { NULL, chptr->chname, me.name, ts, str, NULL }; do_cmd(&me, &me, "TOPIC", 5, p); There's 2 places that prevent this from working, see the attached patch file for details. =] | ||||
| Attached Files | m_topic-IsMe-20180824.diff (713 bytes) | ||||
| 3rd party modules | |||||
|
|
Module devs sometimes use it as a shortcut, but do_cmd() nor any of the command functions (m_xxxx) are meant to be called with &me. You're better off copying the code to set the topic and spread the change, or request a move of such code to a dedicated function that you can call or something. I know it's easy to just apply your patch but as I said, the code is not written (nor security audited) to deal with that so that would raise false expectations. |
|
|
>You're better off copying the code to set the topic and spread the change Yeah I thought about doing just that, but it seemed kinda dirty. :D But if that's actually the way to go then that's cool. =] >but do_cmd() nor any of the command functions (m_xxxx) are meant to be called with &me I'll keep that in mind. What about RunHook though? If I were to copy m_topic's code to set/spread it, would I also need to include RunHook4(HOOKTYPE_TOPIC, &me, &me, chptr, str)? |
|
|
Yes, you hit the nail on the head. Code does not expect this, so this is true for both unrealircd and 3rd party modules :D. You now have the choice to call the hook and risk a crash in some module, or not calling the hook and risking not notifying about a topic change even though some module depends on it :p. Fun eh? ;) |
|
|
Welllllllll in my modules I tend to check IsMe() myself so those shouldn't be an issue. :D But this report is about a very specific use case which I doubt many networks share, so I'm ok with just omitting the hook entirely here. The ported m_chansno does have a hook for LOCAL_TOPIC (it's also the only mod hooking it) but it only accesses sptr->name and not sptr->user or whatever, so that one would keep working fine and dandy regardless. As I said, I'll keep the limitations in mind for future modules. I don't think I'd need to call commands "manually" in conjunction with &me that often anyways (currently doing it nowhere). :D Thanks for the info. ;] |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2018-08-24 22:30 | Gottem | New Issue | |
| 2018-08-24 22:30 | Gottem | File Added: m_topic-IsMe-20180824.diff | |
| 2018-08-25 13:55 | syzop | Note Added: 0020233 | |
| 2018-08-25 13:57 | syzop | Note Edited: 0020233 | |
| 2018-08-25 20:06 | Gottem | Note Added: 0020234 | |
| 2018-08-26 18:14 | syzop | Note Added: 0020235 | |
| 2018-08-27 18:21 | Gottem | Note Added: 0020236 | |
| 2018-09-06 21:11 | syzop | Assigned To | => syzop |
| 2018-09-06 21:11 | syzop | Status | new => closed |
| 2018-09-06 21:11 | syzop | Resolution | open => no change required |
