[Mono-devel-list] [PATCH?] Bugfix for AMD64 stopper plus some general thoughts...

[Willibald Krenn]

Hey!

Ok, before anything else: If this has already been fixed, then please 
ignore this email.

Some while ago, I've already said in an email that I had to patch the 
RReader in RemotingConfiguration.cs to get things working here. At that 
time I did not investigate the problem, but just provided a patch that 
'fixed' the bug. Today, after I switched to my new mono-svn account (rw 
now - yey!) I stumbled into mentioned problem again and decided to see 
if I could fix it.

The problem is specific to AMD64 and is demonstrated by following code 
in RemotingConfiguration.cs:

String xml;
int pos = 0;
'loop' {
	try {
		return (int) xml[pos++];
	}
	catch {
		return -1;
	}
} until -1 is returned

<GeneralThoughts>
I consider this piece of code worth to be contributed to the dustbin of 
history and I will back up my position with a few arguments:
  -> First, it's bad practice to rely on exceptions being thrown for 
events that are perfectly normal and - worse - even expected! Not only 
because exceptions are expensive.
  -> Second, this (and similar) code is not worth to live in the corlib, 
as it will fail and/or lead to security issues when bounds checking is 
not working properly
  -> Third, given that there exist switches in Mini that completely turn 
off bounds checking at all, the code has not the faintest right to live.
</GeneralThoughts>

As already indicated, the AMD64 version of mini has a bug that prevents 
certain bounds checkes from working correctly. Given the asm below

     0x0000002a966ee2e7:     mov    0xffffffffffffffb0(%rbp),%rcx
     0x0000002a966ee2eb:     movslq 0xffffffffffffffac(%rbp),%rax
     0x0000002a966ee2ef:     cmp    %rax,0x10(%rcx)
     0x0000002a966ee2f3:     jbe    0x2a966ee395

- which actually is a String bounds-check  (rcx is ptr to MonoString) - 
you might already see the problem. It's the compare instruction:

(gdb) info registers rcx
rcx            0x779fc0	7839680
(gdb) x/1xg 0x779fd0
0x779fd0:	0x006500480000000b
(gdb)

Got it?
Yes, the length of a MonoString is a guint32 but Mini compares two 
guint64.. So the result is dependand on the next guint32 in memory (== 
AFAIK the string values).


The easiest fix I can provide is attached and does nothing but force any 
OP_X86_COMPARE_MEMBASE_REG to work with 32 bits only. Two reasons why I 
think this is ok:
  -> First, the opcode seems to be used for the bounds-check only. 
(Correct me, if I'm wrong) And all arrays seem to be limited to 2^32 
entries at max.
  -> Second, the X86 in the name supports the 32-bits only approach. If 
a 64 bits compare is needed, it could be named 
OP_AMD64_COMPARE_MEMBASE_REG..

So the one-liner patch is attached to this email; Before going further 
and commiting it, I'd like to have some feedback regarding this..

Thanks & have a nice weekend,
  Willi

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: quickdiff
Url: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20050130/2998e188/attachment.pl