View Issue Details

IDProjectCategoryView StatusLast Update
0004620unrealircdpublic2019-10-27 10:13
ReporterdboyzAssigned Tosyzop 
PrioritynormalSeveritytrivialReproducibilityN/A
Status resolvedResolutionfixed 
Product Version4.0.2 
Target VersionFixed in Version5.0.0-beta1 
Summary0004620: Use proper function instead of do_cmd() in NICK/UID
DescriptionSome of the parameters received from NICK/UID is handled by calling "CMD_FUNC(m_user)" through "do_cmd()". However I found the codes to be logically conflicting and confusing. Essentially a well commented and proper function should be the way to go.

Note: The oddness apply to both NICK and UID although only NICK is shown below.

-i-
"cptr" is found to be a server [1]

-ii-
do_cmd is used to call CMD_FUNC(m_user) [2]

-iii-
However, we have the following condition checking which is _supposed_ to "return ()" because of -i- [3]

-iv-
Also, M_SERVER is not specified in the CommandAdd() function call for CMD_FUNC(m_user) [4]

-v-
Lastly, -iii- is _supposed_ to be denied as per below because of -iv- [5]

References:
[1] - https://github.com/unrealircd/unrealircd/blob/unreal40/src/modules/m_nick.c#L481
[2] - https://github.com/unrealircd/unrealircd/blob/unreal40/src/modules/m_nick.c#L486
[3] - https://github.com/unrealircd/unrealircd/blob/unreal40/src/modules/m_user.c#L73
[4] - https://github.com/unrealircd/unrealircd/blob/unreal40/src/modules/m_user.c#L40
[5] - https://github.com/unrealircd/unrealircd/blob/unreal40/src/parse.c#L376

# Perhaps there is something that I didn't quite understand about do_cmd().
# tl;dr - Again, a well commented and proper function should be used instead of what we currently have here.
TagsNo tags attached.
3rd party modules

Activities

dboyz

2016-04-04 07:37

reporter   ~0019193

As mentioned, the oddness listed above might not be an issue due to my misinterpretation.

To save everyone's time, I decided to clarify a bit on how I interpret the code. That way, we can immediately determine if the oddness is a valid bug or not.

> "cptr" mentioned in -i- is the pointer pointing to aClient struct. In this case, it belongs to the server connected to us. The server is also introducing users to us.

syzop

2016-04-09 09:31

administrator   ~0019213

yeah that calling USER etc is rather ugly. However, cleaning it up is a major operation and risky. I don't see that happening in 4.0.x (now stable). It can happen in a 4.1.x but that is not yet open.

So I suggest to keep this issue open & in the back of our minds for our next list of code cleanups.

As for the direct issue you mention, sorry I haven't walked through your logic. I went for the short tl;dr version :D

syzop

2019-10-27 10:13

administrator   ~0021062

This is now cleaned up in U5.

Issue History

Date Modified Username Field Change
2016-04-04 05:10 dboyz New Issue
2016-04-04 07:37 dboyz Note Added: 0019193
2016-04-09 09:31 syzop Note Added: 0019213
2019-10-27 10:13 syzop Assigned To => syzop
2019-10-27 10:13 syzop Status new => resolved
2019-10-27 10:13 syzop Resolution open => fixed
2019-10-27 10:13 syzop Fixed in Version => 5.0.0-beta1
2019-10-27 10:13 syzop Note Added: 0021062