View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0003954 | unreal | ircd | public | 2010-09-05 01:08 | 2010-09-05 17:13 |
| Reporter | Jobe | Assigned To | |||
| Priority | normal | Severity | trivial | Reproducibility | always |
| Status | closed | Resolution | no change required | ||
| Product Version | 3.2.8 | ||||
| Summary | 0003954: cloak.c: downsample() | ||||
| Description | Not really sure this is a bug, but the downsample() function currently used by src/modules/cloak.c (the cloaking module) produces mathematically incorrect results for the process it employs to downsample MD5 hashes into 32 bit int's. The reason is down to signed char vs unsigned char. Now through some experimenting of my own, simply changing from char to unsigned char in downsample() alone (attached is a diff) solves this issue as demonstrated below: An example of the difference this issue causes is (see attached dstest.c file for the code that produced this): Original: 67bfc0db Unsigned: 68c0c1db A working example of why is: Shift+Bin: a=ffffc200 b=0000c200 Shift+Bin+Add: a=00a8c200 b=00a9c200 Poduced using (where a is char, and b is unsigned char, both set to 0xc2 and 11075584 is 0xa90000): printf("Shift+Bin: a=%08x b=%08x\n", (unsigned int)a << 8, (unsigned int)b << 8); printf("Shift+Bin+Add: a=%08x b=%08x\n", ((unsigned int)a << 8) + 11075584, ((unsigned int)b << 8) + 11075584); I cannot as yet see any performance difference between the two different data types used in this way, but I can see the results are not mathematically the same nor as expected. The down side to the fix is there WILL be a change to the cloaked hosts/IP's generated as a result so it may be worthwhile to allow this "quirk" to remain in place within the 3.2 series. | ||||
| Attached Files | cloak-downsample-unsigned.diff (711 bytes) dstest.c (1,213 bytes) | ||||
| 3rd party modules | |||||
|
|
Sorry this is just a QUICK reply, I did not read your post fully or tested things: UnrealIRCd is compiled with compiler switches that make all 'char' to be 'unsigned char' [*], so there should be no difference, and your patch should not change any behavior or cause different output (in Unreal, with the compile parameters we use, that is). [*] This is -fno-unsigned-char on GCC and <something else> on MSVC++. |
|
|
Yeah I hadn't checked the compiler switches, and have just with further testing found this issue to be invalid. So feel free to close it (I'd close it myself but I dont have access to :P). Was a late at night report and I was tired. So next time ill check things more thoroughly. |
|
|
sure, np. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2010-09-05 01:08 | Jobe | New Issue | |
| 2010-09-05 01:08 | Jobe | File Added: cloak-downsample-unsigned.diff | |
| 2010-09-05 01:08 | Jobe | File Added: dstest.c | |
| 2010-09-05 13:22 | syzop | Note Added: 0016326 | |
| 2010-09-05 16:41 | Jobe | Note Added: 0016327 | |
| 2010-09-05 17:13 | syzop | QA | => Not touched yet by developer |
| 2010-09-05 17:13 | syzop | U4: Need for upstream patch | => No need for upstream InspIRCd patch |
| 2010-09-05 17:13 | syzop | Note Added: 0016328 | |
| 2010-09-05 17:13 | syzop | Status | new => closed |
| 2010-09-05 17:13 | syzop | Resolution | open => no change required |
