0002981: SVSMODE -b and the new cloaked matching ban stuff

(0012019)
Zell (reporter)
2006-06-25 23:17

Thanks for reporting that one Rob =) I thought I was just crazy and / or misconfiguring something IRCd-side. =)

(0012043)
satmd (reporter)
2006-07-03 18:26
edited on: 2006-07-03 18:27

I will probably look into that bug later, but until then (if someone else is going to look at the code):
The SVSMODE should result into 3 checks for each banlist entry: real host, cloaked host (which seems to be missing here!) and vhost. (We cannot assume that we can skip the cloaked host even if we have a vhost enabled.)

(0012048)
tabrisnet (reporter)
2006-07-04 16:45

Just looked at the code, starting at line 196 from m_svsmode.c in 3.2.5

it makes uhost (realhost), vhost (natch), and ihost (IP host), but no chost.

(0012049)
tabrisnet (reporter)
2006-07-04 16:50

Of interest is that GetHost() gets realhost if no umode +x. You could probably shortcircuit the check by checking IsHidden(sptr). Not sure if that would make it any faster or less readable.

meanwhile, the patch to make this add chost or w/e should be trivial. just add a new string var that you use sptr->user->cloakedhost

(0012050)
tabrisnet (reporter)
2006-07-04 17:04
edited on: 2006-07-04 18:08

Uploaded a patch. Compile tested, not runtested.

I have a feeling that this could use some work however. You have basically the same code 3 times, for case b, e, and I. It would be nice if it was consolidated.

something like
int unban_user(chptr, acptr, chmode)

chmode would be a single char, 'b', 'e', or 'I'. And for case 'I' we don't check extbantypes

No doubt similar could be done for clearing ops/voices/etc.

*** Edit b/c we still need chptr, so don't pass banlist, pass chptr and find banlist internally ***

*** Edit rename unban(...) to unban_user(...) Better name suggestions appreciated. maybe UnbanUser(...) ??? ***

(0012052)
tabrisnet (reporter)
2006-07-04 18:49

Here is a first draft patch that implements what I described.
It is also compile-tested only, and assumes the previous patch was applied.

Please forgive if I made style mistakes, I am not completely acquainted with the surrounding code style, and my C is quite rusty.

that and probably these functions should have proper returns, although the values are never checked. Probably return the number of bans affected/removed.

(0012089)
tabrisnet (reporter)
2006-07-27 22:58
edited on: 2006-07-27 23:00

svsmode-reorganize-bans-rev2.diff

Ok, that's a corrected patch, although the two previous patches are now just one. I'm sorry that the previous one was wrong, it was only compile-tested. Today I runtime tested it, found a bug, and fixed it.

Please consider for 3.2.6. Hell, if anybody sees similar issues elsewhere, I'll consider patching those too.

(0012122)
syzop (administrator)
2006-08-02 18:06

I like the idea, will verify the patch later and apply it.

(0012129)
syzop (administrator)
2006-08-03 07:14
edited on: 2006-08-03 07:15

Should be fixed in .551:
- Fixed SVSMODE -b [user] not always removing all bans (specificly, bans on the cloaked
host when you have a vhost), a code cleanup was also done. Based on patch from tabrisnet.
Reported by Rob (0002981).

If someone could verify this fixes it, that would be nice :)

(as you can see I had to modify your patch a bit, otherwise it would cause heap corruption (linked list fun), and had to check for availability of IP/etc as well for users)

(0012144)
tabrisnet (reporter)
2006-08-03 19:41
edited on: 2006-08-03 19:45

What kind of verification do you want? I know that the patch I did originally worked. And I have applied it to my production network.

Additionally, I just did a raw svsmode, and used it to unban myself from a channel, where my cloakhost was banned and my vhost was set.

(0012150)
syzop (administrator)
2006-08-04 06:17

Well, verification that it works, of course :P.

Your original patch was very dangerous and will corrupt the heap very easily.

Ah well, I'll just mark this issue as fixed, quite sure it works ok and we'll have it tested during the testing rounds anyway.

Thanks.

Issue History
Date Modified	Username	Field	Change
2006-06-21 14:36	Rob	New Issue
2006-06-21 14:43	Rob	Issue Monitored: Rob
2006-06-21 15:20	tabrisnet	Issue Monitored: tabrisnet
2006-06-21 17:23	syzop	Status	new => acknowledged
2006-06-21 17:24	syzop	Relationship added	child of 0002936
2006-06-25 23:15	Zell	Issue Monitored: Zell
2006-06-25 23:17	Zell	Note Added: 0012019
2006-07-03 18:05	satmd	Issue Monitored: satmd
2006-07-03 18:26	satmd	Note Added: 0012043
2006-07-03 18:27	satmd	Note Edited: 0012043
2006-07-04 16:45	tabrisnet	Note Added: 0012048
2006-07-04 16:50	tabrisnet	Note Added: 0012049
2006-07-04 16:59	tabrisnet	File Added: svsmode-ban-cloakhost.diff
2006-07-04 17:04	tabrisnet	Note Added: 0012050
2006-07-04 18:07	tabrisnet	Note Edited: 0012050
2006-07-04 18:08	tabrisnet	Note Edited: 0012050
2006-07-04 18:47	tabrisnet	File Added: svsmode-reorg-bans.diff
2006-07-04 18:49	tabrisnet	Note Added: 0012052
2006-07-21 10:00	Monk	Issue Monitored: Monk
2006-07-27 22:56	tabrisnet	File Added: svsmode-reorganize-bans-rev2.diff
2006-07-27 22:58	tabrisnet	Note Added: 0012089
2006-07-27 23:00	tabrisnet	Note Edited: 0012089
2006-08-02 18:06	syzop	Note Added: 0012122
2006-08-03 07:14	syzop	Note Added: 0012129
2006-08-03 07:15	syzop	Note Edited: 0012129
2006-08-03 19:41	tabrisnet	Note Added: 0012144
2006-08-03 19:45	tabrisnet	Note Edited: 0012144
2006-08-04 06:17	syzop	Status	acknowledged => resolved
2006-08-04 06:17	syzop	Fixed in Version	=> 3.2.6
2006-08-04 06:17	syzop	Resolution	open => fixed
2006-08-04 06:17	syzop	Assigned To	=> syzop
2006-08-04 06:17	syzop	Note Added: 0012150
2007-03-26 03:20	tabrisnet	Issue End Monitor: tabrisnet
2007-04-27 05:28	stskeeps	Relationship added	related to 0002469

Notes
(0012019) Zell (reporter) 2006-06-25 23:17	Thanks for reporting that one Rob =) I thought I was just crazy and / or misconfiguring something IRCd-side. =)

(0012043) satmd (reporter) 2006-07-03 18:26 edited on: 2006-07-03 18:27	I will probably look into that bug later, but until then (if someone else is going to look at the code): The SVSMODE should result into 3 checks for each banlist entry: real host, cloaked host (which seems to be missing here!) and vhost. (We cannot assume that we can skip the cloaked host even if we have a vhost enabled.)

(0012048) tabrisnet (reporter) 2006-07-04 16:45	Just looked at the code, starting at line 196 from m_svsmode.c in 3.2.5 it makes uhost (realhost), vhost (natch), and ihost (IP host), but no chost.

(0012049) tabrisnet (reporter) 2006-07-04 16:50	Of interest is that GetHost() gets realhost if no umode +x. You could probably shortcircuit the check by checking IsHidden(sptr). Not sure if that would make it any faster or less readable. meanwhile, the patch to make this add chost or w/e should be trivial. just add a new string var that you use sptr->user->cloakedhost

(0012050) tabrisnet (reporter) 2006-07-04 17:04 edited on: 2006-07-04 18:08	Uploaded a patch. Compile tested, not runtested. I have a feeling that this could use some work however. You have basically the same code 3 times, for case b, e, and I. It would be nice if it was consolidated. something like int unban_user(chptr, acptr, chmode) chmode would be a single char, 'b', 'e', or 'I'. And for case 'I' we don't check extbantypes No doubt similar could be done for clearing ops/voices/etc. * Edit b/c we still need chptr, so don't pass banlist, pass chptr and find banlist internally * * Edit rename unban(...) to unban_user(...) Better name suggestions appreciated. maybe UnbanUser(...) ??? *

(0012052) tabrisnet (reporter) 2006-07-04 18:49	Here is a first draft patch that implements what I described. It is also compile-tested only, and assumes the previous patch was applied. Please forgive if I made style mistakes, I am not completely acquainted with the surrounding code style, and my C is quite rusty. that and probably these functions should have proper returns, although the values are never checked. Probably return the number of bans affected/removed.

(0012089) tabrisnet (reporter) 2006-07-27 22:58 edited on: 2006-07-27 23:00	svsmode-reorganize-bans-rev2.diff Ok, that's a corrected patch, although the two previous patches are now just one. I'm sorry that the previous one was wrong, it was only compile-tested. Today I runtime tested it, found a bug, and fixed it. Please consider for 3.2.6. Hell, if anybody sees similar issues elsewhere, I'll consider patching those too.

(0012122) syzop (administrator) 2006-08-02 18:06	I like the idea, will verify the patch later and apply it.

(0012129) syzop (administrator) 2006-08-03 07:14 edited on: 2006-08-03 07:15	Should be fixed in .551: - Fixed SVSMODE -b [user] not always removing all bans (specificly, bans on the cloaked host when you have a vhost), a code cleanup was also done. Based on patch from tabrisnet. Reported by Rob (0002981). If someone could verify this fixes it, that would be nice :) (as you can see I had to modify your patch a bit, otherwise it would cause heap corruption (linked list fun), and had to check for availability of IP/etc as well for users)

(0012144) tabrisnet (reporter) 2006-08-03 19:41 edited on: 2006-08-03 19:45	What kind of verification do you want? I know that the patch I did originally worked. And I have applied it to my production network. Additionally, I just did a raw svsmode, and used it to unban myself from a channel, where my cloakhost was banned and my vhost was set.

(0012150) syzop (administrator) 2006-08-04 06:17	Well, verification that it works, of course :P. Your original patch was very dangerous and will corrupt the heap very easily. Ah well, I'll just mark this issue as fixed, quite sure it works ok and we'll have it tested during the testing rounds anyway. Thanks.