View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0006013 | unreal | ircd | public | 2021-11-28 20:16 | 2021-12-01 08:54 |
| Reporter | progval | Assigned To | k4be | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | assigned | Resolution | open | ||
| Product Version | 6.0.0-rc1 | ||||
| Summary | 0006013: RPL_MONONLINE is (re)sent on nick case change | ||||
| Description | On Unreal 5 (and all other IRCds I could find), when MONITORing "qux" and there is already a user with that name, "qux" changing their name to "QUX" does not trigger a RPL_MONONLINE. However, on Unreal 6, it does. IMO this is invalid, because the user doesn't really become online, as they were already using an equivalent nick. | ||||
| Steps To Reproduce | monitor "qux": 1 -> S: MONITOR + qux Make a client change their name to "qux": 2 -> S: NICK baz 2 -> S: USER username * * :Realname [...] 2 -> S: NICK qux S -> 2: :baz NICK :qux S -> 1: :My.Little.Server 730 bar :qux!~username@localhost Make the same client change to QUX: 2 -> S: NICK QUX S -> 2: :qux NICK :QUX S -> 1: :My.Little.Server 730 bar :QUX!~username@localhost | ||||
| Additional Information | Server logs: [info] Client connecting: bar (~username@localhost) [127.0.0.1] [vhost: localhost] [class: clients] [reputation: 0] [security-groups: unknown-users] [info] Client connecting: baz (~username@localhost) [127.0.0.1] [vhost: localhost] [class: clients] [reputation: 0] [security-groups: unknown-users] [info] Client baz!~username@localhost has changed their nickname to qux [info] Client qux!~username@localhost has changed their nickname to QUX | ||||
| Tags | No tags attached. | ||||
| 3rd party modules | |||||
|
|
Just to be clear: there is no MONITOR in 5.x, or at least not in official UnrealIRCd :) I think this happens because k4be uses the same code for WATCH. WATCH also has this behavior on nick-change, in 5.x too. That doesn't make it "good" but just saying. k4be: you agree with what progval says? And if so.... you wrote this code, what would you need to fix it? from first sight I think you would want a const char *oldnick in the post-nickchange-hook, am I right? |
|
|
> Just to be clear: there is no MONITOR in 5.x, or at least not in official UnrealIRCd :) Oh, my bad. My script just skipped MONITOR entirely in v5, so I didn't get any error. sorry |
|
|
I think i ran into some problems when trying to pass the old nick in HOOKTYPE_POST_LOCAL_NICKCHANGE argument list. Decided to leave it for later (:D). It can be solved with using moddata to keep the old name between "normal" and "POST" hook calls. WATCH is different in that it does send the "logged out" event too, and MONITOR does not. I don't know whether the issue will cause any problems with clients (memory leaks? duplicate users on "friends" lists?). The MONITOR spec does not mention this case at all. I think this should be fixed, either by re-enabling RPL_MONOFFLINE, or by omitting RPL_MONONLINE. |
|
|
Ok no problem, I'll change the hook. Better to do it now during RC than (not) doing it during stable. Then at least the information is available without too much trouble (eg needing to use moddata). Will leave it up to you and the MONITOR-people to decide what the correct behavior is (and thus, if you will actually use oldnick at all). |
|
|
Ok, 'oldnick' is now available in the post nickchange hooks. https://github.com/unrealircd/unrealircd/commit/4af7a541f8cd9739c2e9f8dc7847aa41941293d8 > I think this should be fixed, either by re-enabling RPL_MONOFFLINE, or by omitting RPL_MONONLINE. I agree. I now made it ignore it in both events for MONITOR. https://github.com/unrealircd/unrealircd/commit/8c8b4279b85058ff0c59b5adab08bfaa7b1a56c4 That seems like your initial idea (hence the check already being there in 1 out of 2). And also because if I add it now then that code gets tested already. I don't like to (potentially) add new code last-minute right before 6.0.0 release. If the MONITOR people and/or you say it should not be like that, then you simply remove the 2x2 lines, which requires no further (crash)testing. I'm going to leave this item open and assigned to you k4be, until you know what is deemed the "correct" thing :D. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2021-11-28 20:16 | progval | New Issue | |
| 2021-11-29 17:32 | syzop | Note Added: 0022215 | |
| 2021-11-29 17:32 | syzop | Assigned To | => k4be |
| 2021-11-29 17:32 | syzop | Status | new => assigned |
| 2021-11-29 18:42 | progval | Note Added: 0022225 | |
| 2021-11-30 01:02 | k4be | Note Added: 0022228 | |
| 2021-11-30 06:42 | syzop | Note Added: 0022231 | |
| 2021-12-01 08:53 | syzop | Note Added: 0022232 | |
| 2021-12-01 08:54 | syzop | Note Edited: 0022232 |
