Bug 11435 – Pushing indirect ref to byte or short can read beyond edge of valid memory

Created attachment 1285 Failing code, insert into phobos tree, modified the makefiles, and run the unit test runner The code I'll attach to this bug report is extracted from a phobos pull request, it fails non-deterministically on the 32 bit auto-test machines when running release mode unit tests. I've reduced it as much as I can using the auto-test as it doesn't fail on my personal machine. It fails most consistently on win32 and freebsd 32 machines and has at some point in my testing, failed on all the 32bit auto-test machines. From what I've gathered from debugging, it fails on line 73 in the x.init(v) call, on the 30'th iteration of that second for loop. I've used the following pull request to test and reduce the code, but I am stuck: https://github.com/D-Programming-Language/phobos/pull/1670 In the pull request code, I've included modifications to the code that made the bug disappear.

Very very nice. Have you been unable to reproduce locally, or do you just have no access to 32 bit machines? I had tried to reproduce locally before, but failed. I'll try again with your reduced code though. I couldn't help noticing this line though (and am surprised it is actually in a nobug1 block): ptr = cast(size_t*)GC.realloc(ptr, newdim*size_t.sizeof, GC.BlkAttr.NO_SCAN | GC.BlkAttr.APPENDABLE); This is *wrong*. Really, the mode "APPENDABLE" really shouldn't even be publicly documented. The problem is that (re)alloc (and friends) will return the "raw" memory block, including the parts reserved to define the block's currently used length, regardless of the blocks' attributes. This leads to the inevitable clobbering of said appendable data. Relevent issue: http://d.puremagic.com/issues/show_bug.cgi?id=10589 (also related) http://d.puremagic.com/issues/show_bug.cgi?id=9092 That said, "nobug1" is (afaik) the only block that does any manual memory management, so I don't think the issue comes from that (but I think it is worth mentioning).

(In reply to comment #1) > Very very nice. > > Have you been unable to reproduce locally, or do you just have no access to 32 > bit machines? I had tried to reproduce locally before, but failed. I'll try > again with your reduced code though. > > I couldn't help noticing this line though (and am surprised it is actually in a > nobug1 block): > ptr = cast(size_t*)GC.realloc(ptr, newdim*size_t.sizeof, GC.BlkAttr.NO_SCAN | > GC.BlkAttr.APPENDABLE); > > This is *wrong*. Really, the mode "APPENDABLE" really shouldn't even be > publicly documented. The problem is that (re)alloc (and friends) will return > the "raw" memory block, including the parts reserved to define the block's > currently used length, regardless of the blocks' attributes. This leads to the > inevitable clobbering of said appendable data. That flag did not seem to affect the behaviour. Maybe I should remove it so it does not become a distraction. I do not have any 32 bit dev machines set up. It did fail on the linux 64/32 auto-tester, but generating 32 bit code locally did not fail on my 64bit machine.

(In reply to comment #1) > [Snip] Ok I let it run with version = nobug1 without the APPENDABLE flag, and it failed, so I've removed that section altogether since it must have just hacked around the issue somehow.

(In reply to monarchdodra from comment #1) > > Have you been unable to reproduce locally, or do you just have no access to > 32 bit machines? I had tried to reproduce locally before, but failed. I'll > try again with your reduced code though. Today I set up a FreeBSD VM (GhostBSD via virtual box,) and I can finally reproduce the bug.

I've managed to reduce it to a test that consistently fails. The reduced test case is posix only (posix with MAP_ANON extension,) but the bug manifests itself on all 32 bit x86 platforms. Disassembly snippet: Here is part of the loop in S.foo(), DMD creates a 4 byte read on <+85>, but it is only valid to read one byte. This causes the segfault. ebx is the loop index and ecx is the pointer to the array. <+80>: mov -0x4(%ebp),%ecx <+83>: mov %esi,%eax => <+85>: pushl (%ebx,%ecx,1) <+88>: push %ebx <+89>: call 0x8070f70 <_D6_114351S13opIndexAssignMFbkZb> <+94>: inc %ebx <+95>: cmp 0x8(%ebp),%ebx <+98>: jb 0x8070f41 <_D6_114351S3fooMFAbZv+65>

Created attachment 1358 Posix reduced test

I can confirm this is an issue on OSX as well, $ dmd -g -m32 -O _11435.d $ ./_11435 249000 Bus error: 10

I managed to work around this issue by modifying the following code from src/backend/cod1.c: (line ~3573) if (sz <= REGSIZE) { // Watch out for single byte quantities being up // against the end of a segment or in memory-mapped I/O if (!(config.exe & EX_flat) && szb == 1) break; goto L1; // can handle it with loadea() } I changed the condition to: if (szb < REGSIZE) break; I did not test further fixes since this was good enough for me. This should be enough information to create a "proper" fix.

Windows test case: import core.sys.windows.windows; import core.stdc.string; extern(C) int printf(in char*, ...); alias T = byte; void fun(T c, T b, T a) { printf("%d %d %d\n", a, b, c); } void abc(T[] b, size_t index) { fun(b[index+1], b[index+2], b[index+3]); } void main() { auto p = VirtualAlloc(null, 4096, MEM_COMMIT, PAGE_EXECUTE_READWRITE); assert(p); memset(p, 0, 4096); abc((cast(T*)(p + 4090))[0..4], 0); }

And the same thing for short (I think) import core.sys.windows.windows; import core.stdc.string; extern(C) int printf(in char*, ...); alias T = short; void fun(T c, T b, int v) { printf("%d %d\n", b); } void abc(T[] b, size_t index) { fun(b[0], b[1], 0); } void main() { auto p = VirtualAlloc(null, 4096, MEM_COMMIT, PAGE_EXECUTE_READWRITE); assert(p); memset(p, 0, 4096); auto px = (cast(T*)(p + 4096 - 2 * T.sizeof)); printf("%p\n", px+1); abc(px[0..2], 0); }

(In reply to yebblies from comment #11) > https://github.com/D-Programming-Language/dmd/pull/3806 I can't reproduce this on win32 or linux32/64 any more, most likely due to this: https://github.com/D-Programming-Language/dmd/pull/5111 Can anyone reproduce the failure on their system?

(In reply to safety0ff.bugz from comment #8) > I managed to work around this issue by modifying the following code from > src/backend/cod1.c: (line ~3573) This code indeed is wrong. Will fix.

@WalterBright created dlang/dmd pull request #11933 "fix Issue 11435 - Pushing indirect ref to byte or short can read beyo…" fixing this issue: - fix Issue 11435 - Pushing indirect ref to byte or short can read beyond edge of valid memory https://github.com/dlang/dmd/pull/11933

dlang/dmd pull request #11933 "fix Issue 11435 - Pushing indirect ref to byte or short can read beyo…" was merged into master: - 778476ea3e9de76156806e0841cda838b21786e8 by Walter Bright: fix Issue 11435 - Pushing indirect ref to byte or short can read beyond edge of valid memory https://github.com/dlang/dmd/pull/11933