Created attachment 188355 [details] Callstack of memory overwrite issue Below functions of Math object causes memory overwrite for big numbers (greater than 2^31) on 32bit platforms (i386, arm) abs() ceil() exp() floor() log() round() sqrt() Valgrind reports: ==32656== Thread 1: ==32656== Invalid write of size 8 ==32656== at 0x804142B: ??? ==32656== by 0x56E30E9: JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) (in /home/wojtek/Projekty/webkit/webkit.org/WebKit/WebKitBuild/Release/lib/libewebkit2.so.0.1.0) ==32656== by 0x570F58D: JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) (in /home/wojtek/Projekty/webkit/webkit.org/WebKit/WebKitBuild/Release/lib/libewebkit2.so.0.1.0) ==32656== by 0x4C0F210: WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*) (in /home/wojtek/Projekty/webkit/webkit.org/WebKit/WebKitBuild/Release/lib/libewebkit2.so.0.1.0) ==32656== by 0xA4A44B7: ??? ==32656== Address 0xbeb95b44 is just below the stack ptr. To suppress, use: --workaround-gcc296-bugs=yes Full gdb backtrace attached. The issue is not reproducible when JIT is disabled. Example JavaScript code that causes memory overwrite: <script> var x = Math.floor(100000000000000000000000000000000000000000); </script>
I've found that "memory overwrite" is done in returnDouble function in SavaScriptCore/jit/SpecializedThunkJIT.h file.
Created attachment 189308 [details] Location of memory overwrite This patch only shows the location of "memory overwrite". I have a doubts if this overwrite is a real problem or just false alarm that's why this patch isn't designed for merge. Could someone take a look closer on this problem?
Created attachment 190067 [details] Location of memory overwrite Correct version - mitigate memory overwrite
Comment on attachment 190067 [details] Location of memory overwrite View in context: https://bugs.webkit.org/attachment.cgi?id=190067&action=review > Source/JavaScriptCore/jit/SpecializedThunkJIT.h:109 > - storeDouble(src, Address(stackPointerRegister, -(int)sizeof(double))); > - loadPtr(Address(stackPointerRegister, OBJECT_OFFSETOF(JSValue, u.asBits.tag) - sizeof(double)), regT1); > - loadPtr(Address(stackPointerRegister, OBJECT_OFFSETOF(JSValue, u.asBits.payload) - sizeof(double)), regT0); > + push(regT2); > + push(regT2); > + storeDouble(src, Address(stackPointerRegister, +(int)sizeof(double))); > + loadPtr(Address(stackPointerRegister, OBJECT_OFFSETOF(JSValue, u.asBits.tag) + sizeof(double)), regT1); > + loadPtr(Address(stackPointerRegister, OBJECT_OFFSETOF(JSValue, u.asBits.payload) + sizeof(double)), regT0); > Jump lowNonZero = branchTestPtr(NonZero, regT1); > Jump highNonZero = branchTestPtr(NonZero, regT0); > + pop(regT2); > + pop(regT2); This looks wrong; you seem to be jumping over the pops. This will leave the stack in a weird state. I'm not sure if this is a real bug, either. It ought to be safe to store at a negative stack offset, in most platforms. Plus, I don't like the implications for performance of the thunk: you're doing some extra pushes and pops. But if it's a real bug, you could have implemented this by changing the offset at which we store the double. The JITStackFrame::args should be reusable here, since we're not in the middle of a JITStub call.
(In reply to comment #4) > This looks wrong; you seem to be jumping over the pops. This will leave the stack in a weird state. > > I'm not sure if this is a real bug, either. It ought to be safe to store at a negative stack offset, in most platforms. > > Plus, I don't like the implications for performance of the thunk: you're doing some extra pushes and pops. > > But if it's a real bug, you could have implemented this by changing the offset at which we store the double. The JITStackFrame::args should be reusable here, since we're not in the middle of a JITStub call. Indeed I've made a mistake in pop/jump order that obscures the idea. Sorry for that. But in general I was wondering if there is any chance that after many software and hardware optimizations on CPU cache L1/L2 level (where SP could be used as a reference of "untouched" memory after it) the memory located just after stack pointer in CPU cache could be used for a completely different memory page - what would be reason of seriously bug. Please keep in mind that the patch isn't prepared for merge. It only serves to bare the problem.
(In reply to comment #5) > (In reply to comment #4) > > > This looks wrong; you seem to be jumping over the pops. This will leave the stack in a weird state. > > > > I'm not sure if this is a real bug, either. It ought to be safe to store at a negative stack offset, in most platforms. > > > > Plus, I don't like the implications for performance of the thunk: you're doing some extra pushes and pops. > > > > But if it's a real bug, you could have implemented this by changing the offset at which we store the double. The JITStackFrame::args should be reusable here, since we're not in the middle of a JITStub call. > > Indeed I've made a mistake in pop/jump order that obscures the idea. Sorry for that. > But in general I was wondering if there is any chance that after many software and hardware optimizations on CPU cache L1/L2 level (where SP could be used as a reference of "untouched" memory after it) the memory located just after stack pointer in CPU cache could be used for a completely different memory page - what would be reason of seriously bug. > Please keep in mind that the patch isn't prepared for merge. It only serves to bare the problem. http://en.wikipedia.org/wiki/Red_zone_(computing) It is safe to modify at least 8 bytes (and as much as 128 on x86_64) beyond the stack pointer without messing with the stack pointer itself. Nite that if the 8 bytes beyond the stack were on a different page then either that page is already reserved for the stack (in which case we're fine) or its not - in which case anything we try to do will overflow the stack and crash the program. Calling Math.floor is the least of our worries if the JITStackFrame is at the extreme bottom of the stack and we're about to run out.
Looks like false alarm in this case.