View Issue Details

IDProjectCategoryView StatusLast Update
0004213unrealircdpublic2015-07-10 13:06
ReporternenolodAssigned Tosyzop 
PrioritynormalSeverityminorReproducibilityN/A
Status resolvedResolutionfixed 
Product Version 
Target Version3.4-alpha1Fixed in Version3.4-beta1 
Summary0004213: parv[0] usage needs to be fixed
DescriptionAnywhere we're using parv[0], we should use sptr->name or sptr->id.

We also need a macro to choose sptr->id over sptr->name if an ID is present (which will be used for the TS6-ish messages).

Using parv[0] means we do not have control over what we're sending over the wire, which can desync non-SID servers.

This bug must be fully resolved before we can go beta.
TagsNo tags attached.
3rd party modules

Relationships

related to 0004215 closed Unreal 3.4 alpha2 blockers 
child of 0004212 resolvedsyzop TS6-ish server protocol metabug 

Activities

falconkirtaran

2013-05-24 13:18

reporter  

4213_sledgehammer_parv_change.diff (128,646 bytes)

falconkirtaran

2013-05-24 13:19

reporter   ~0017662

Sledgehammer-like patch to change this. Similar to the checked string operations patch, it will likely introduce at least one bug which we will turn up if we do detailed testing ;)

syzop

2013-05-26 11:02

administrator   ~0017685

Shouldn't the sledgehammer-like patch use the macro you mentioned?
I presume we should use UID everywhere possible?
Or does your sledgehammer not provide that capability ;p

You can also (upstream) make parv[0] = NULL / 0xDEADBEEF / something other nasty so you can easily catch anyone using it...

falconkirtaran

2013-05-26 13:07

reporter   ~0017690

I'm not nenolod. Anyway, it should definitely use the macro in some cases, which we might want to review. I may amend this patch to do that if I have time.

syzop

2013-05-26 15:10

administrator   ~0017691

Ok, good :)

Don't understand your "I'm not nenolod" though. Perhaps my 'upstream' term confused you? Sorry. I meant setting para[0] to NULL in parse(). Again, if you want a simple way to trace any issues, I'm not saying it's a great idea in other circumstances.

falconkirtaran

2013-05-26 21:32

reporter   ~0017693

Of course. I merely meant that I didn't mention the macro you're talking about at all.

syzop

2015-07-10 13:05

administrator   ~0018470

Done.

https://github.com/unrealircd/unrealircd/commit/e8dfb284a1edf3da0bdea230a61ce8a960a2480a
https://github.com/unrealircd/unrealircd/commit/0c516abc76341db0aae43bb9870241ceb954eab7

commit e8dfb284a1edf3da0bdea230a61ce8a960a2480a
Author: Bram Matthys <syzop@vulnscan.org>
Date: Fri Jul 10 12:17:05 2015 +0200

    Replace parv[0] with sptr->name. Don't use parv[0] anymore.
    I went through all 500+ of them by hand as to avoid introducing bugs... we'll see ;)

commit 0c516abc76341db0aae43bb9870241ceb954eab7
Author: Bram Matthys <syzop@vulnscan.org>
Date: Fri Jul 10 12:29:07 2015 +0200

    You can now no longer use parv[0]. Doing so will lead to a crash, this is intentional. Use sptr->name instead.
    No UnrealIRCd code reads from parv[0] anymore.
    Perhaps later, after a few stable versions, we'll turn this into something more useful. Or not. But not soon.

Issue History

Date Modified Username Field Change
2013-05-23 10:10 nenolod New Issue
2013-05-23 10:11 nenolod Relationship added child of 0004212
2013-05-24 13:18 falconkirtaran File Added: 4213_sledgehammer_parv_change.diff
2013-05-24 13:19 falconkirtaran Note Added: 0017662
2013-05-25 00:37 nenolod Relationship added related to 0004215
2013-05-26 11:02 syzop Note Added: 0017685
2013-05-26 13:07 falconkirtaran Note Added: 0017690
2013-05-26 15:10 syzop Note Added: 0017691
2013-05-26 21:32 falconkirtaran Note Added: 0017693
2014-03-14 01:14 peterkingalexander Issue cloned: 0004304
2015-07-10 13:05 syzop Note Added: 0018470
2015-07-10 13:05 syzop Status new => resolved
2015-07-10 13:05 syzop Fixed in Version => 3.4-beta1
2015-07-10 13:05 syzop Resolution open => fixed
2015-07-10 13:05 syzop Assigned To => syzop