View Issue Details

IDProjectCategoryView StatusLast Update
0006013unrealircdpublic2021-12-01 08:54
Reporterprogval Assigned Tok4be  
PrioritynormalSeverityminorReproducibilityalways
Status assignedResolutionopen 
Product Version6.0.0-rc1 
Summary0006013: RPL_MONONLINE is (re)sent on nick case change
DescriptionOn 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 Reproducemonitor "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 InformationServer 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
TagsNo tags attached.
3rd party modules

Activities

syzop

2021-11-29 17:32

administrator   ~0022215

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?

progval

2021-11-29 18:42

reporter   ~0022225

> 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

k4be

2021-11-30 01:02

developer   ~0022228

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.

syzop

2021-11-30 06:42

administrator   ~0022231

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).

syzop

2021-12-01 08:53

administrator   ~0022232

Last edited: 2021-12-01 08:54

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.

Issue History

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