Closed
Bug 564739
Opened 14 years ago
Closed 14 years ago
test_jsctypes.js will crash on Windows x64 since ffi_call_win64 overrun memory
Categories
(Core :: js-ctypes, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: m_kato, Assigned: dwitte)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 4 obsolete files)
6.59 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
returnValue allocates 1 byte when type is bool. But ffi_call_win64 writes 8 bytes for returnValue in ret_uint8/ret_sint8. So this causes heap corruption.
Reporter | ||
Updated•14 years ago
|
Summary: test_jsctypes.js will crash on Windows x64 build due to libffi update → test_jsctypes.js will crash on Windows x64 since ffi_call_win64 overrun memory
Reporter | ||
Comment 1•14 years ago
|
||
Reporter | ||
Comment 2•14 years ago
|
||
Comment on attachment 444375 [details] [diff] [review] patch Dan, should I fix libffi instead of?
Attachment #444375 -
Flags: review?(dwitte)
Assignee | ||
Comment 3•14 years ago
|
||
Hmm. I ran into this problem with the x86/win32 code as well. It extended a byte return value into a word. The problem is, I'm not sure (right now) whether it's assumed in the libffi API that return values should always have word size. I think it might be. For little-endian arches, it doesn't matter, and what you have is fine. But for big-endian, there'll be an address offset. Want to check? ppc and arm would be good examples. (I *think* arm is big-endian by convention, though technically it's bi-endian.)
Reporter | ||
Comment 4•14 years ago
|
||
x86/win32 code only copies 1 byte for return value when int8 size. Same problem is possible on UNIX x86-64 ABI. When int8/int16 case, ffi_call_unix64 in unix64.S copies 32bit value to memory for return value. I think that it may detect using valgrind, so I will setup linux x86-64 box with valgrind. Also, arm supports both endian. Although I have arm linux board using cortex-a8, this uses little endian version (armv7l) of Linux. Usual distributions of Linux such as Ubuntu use little endian. This problem depends on ffi_call implementation each cpu/os.
Reporter | ||
Comment 5•14 years ago
|
||
When I use valgrind with --tool=memcheck on x86-64 Linux, it detects invalid write. Most memory allocator will allocate pointer size (On x86-64, 8 bytes) even if request is 1 bytes. So I believe that this is debug heap only. TEST-PASS | /home/makoto/Development/objdir64/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js | [run_bool_tests : 807] ctypes.bool(true) == ctypes.bool(true) ==1060== Invalid write of size 8 ==1060== at 0x6DD1420: ffi_call_unix64 (unix64.S:116) ==1060== by 0x6DD1153: ffi_call (ffi64.c:484) ==1060== by 0x6DC10F6: js::ctypes::FunctionType::Call(JSContext*, JSObject*, unsigned int, long*, long*) (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x6CC8A8F: js_Invoke (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x6CB9B73: js_Interpret (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x6CC828C: js_Execute (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x6C68C94: JS_EvaluateUCScriptForPrincipals (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x6C6F181: JS_EvaluateScriptForPrincipals (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x404E79: main (in /home/makoto/Development/objdir64/dist/bin/xpcshell) ==1060== Address 0x147f2680 is 0 bytes inside a block of size 1 alloc'd ==1060== at 0x4C2263F: operator new[](unsigned long) (vg_replace_malloc.c:264) ==1060== by 0x6DCACCB: js::ctypes::AutoValue::SizeToType(JSContext*, JSObject*) (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x6DC11C6: js::ctypes::FunctionType::Call(JSContext*, JSObject*, unsigned int, long*, long*) (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x6CC8A8F: js_Invoke (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x6CB9B73: js_Interpret (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x6CC828C: js_Execute (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x6C68C94: JS_EvaluateUCScriptForPrincipals (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x6C6F181: JS_EvaluateScriptForPrincipals (in /home/makoto/Development/objdir64/js/src/libmozjs.so) ==1060== by 0x404E79: main (in /home/makoto/Development/objdir64/dist/bin/xpcshell) ==1060== TEST-PASS | /home/makoto/Development/objdir64/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js | [run_single_abi_tests : 668] true == true
Assignee | ||
Comment 6•14 years ago
|
||
The x86/win32 code only does 1 byte because when I wrote it, that made the most sense -- but given the precedent elsewhere, it might not anymore... It looks like ppc stores the whole return word (from r3): http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/libffi/src/powerpc/darwin.S#138 I think tests pass on ppc though, which I don't understand.
Reporter | ||
Comment 7•14 years ago
|
||
Dan, can you test it on valgrind? Although Linux x86-64 passes xpcshell-tests, valgrind detects error with --disable-jemalloc. Also, it will detect "Invalid write" error on PPC if using valgrind. This depends on memory allocator.
Assignee | ||
Comment 8•14 years ago
|
||
Yeah. We absolutely shouldn't depend on allocator; the answer here is either a) audit and possibly fix each assembly implementation in libffi to write the correct size; b) coerce the return value via an 'ffi_arg' intermediary into the actual CData buffer. We should just do b), since a) will be bad for libffi compatibility. It's been raised on the mailing list and should be something we can fix for a libffi 4.0.0 release. This same problem applies to closure code, I believe.
Reporter | ||
Comment 9•14 years ago
|
||
Comment on attachment 444375 [details] [diff] [review] patch cancel review. This is possible on other OS/CPU. I will care for others.
Attachment #444375 -
Flags: review?(dwitte)
Reporter | ||
Comment 10•14 years ago
|
||
Attachment #444375 -
Attachment is obsolete: true
Reporter | ||
Updated•14 years ago
|
Attachment #447979 -
Flags: review?(dwitte)
Assignee | ||
Comment 11•14 years ago
|
||
Comment on attachment 447979 [details] [diff] [review] patch v2 Won't this still break on big endian arches when converting the return value word to a smaller integral type, because the pointer offsets will be wrong? http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#1541
Reporter | ||
Comment 13•14 years ago
|
||
(In reply to comment #11) > Comment on attachment 447979 [details] [diff] [review] > patch v2 > > Won't this still break on big endian arches when converting the return value > word to a smaller integral type, because the pointer offsets will be wrong? > > http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#1541 This bug is allocation size issue, not endian. If big endian, pointer address is constant even if int64, int or bool.
Assignee | ||
Comment 14•14 years ago
|
||
(In reply to comment #13) > This bug is allocation size issue, not endian. If big endian, pointer address > is constant even if int64, int or bool. Huh? That's only true for little endian. int i = 0x11223344; for (int j = 0; j < 4; ++j) printf("%x ", int(*((char*)(&i) + j))); should give you 44 33 22 11 on little endian and 11 22 33 44 on big endian, right? So if the ppc code writes an entire register into an address, the least significant byte will be stored at (address + 3), such that when we cast that address to char* and dereference it we'll get the most significant byte instead. Do you disagree?
Assignee | ||
Comment 16•14 years ago
|
||
Comment on attachment 447979 [details] [diff] [review] patch v2 r- since this doesn't solve the entire problem.
Attachment #447979 -
Flags: review?(dwitte) → review-
Assignee | ||
Comment 17•14 years ago
|
||
Assignee: m_kato → dwitte
Attachment #447979 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #459050 -
Flags: review?(benjamin)
Assignee | ||
Comment 18•14 years ago
|
||
Attachment #459051 -
Flags: review?(benjamin)
Assignee | ||
Comment 19•14 years ago
|
||
Want this for Fx4 -- overrunning memory is bad.
blocking2.0: --- → ?
Comment 20•14 years ago
|
||
Indeed, sync is already crashing on Linux64 because of this.
blocking2.0: ? → beta3+
Updated•14 years ago
|
Attachment #459050 -
Flags: review?(benjamin) → review+
Comment 21•14 years ago
|
||
Comment on attachment 459051 [details] [diff] [review] part 2: fix ffi_closure usage Pretty sure the assert about sizeof(long) == sizeof(ffi_arg) is incorrect. But it's not clear why you needed it.
Attachment #459051 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 22•14 years ago
|
||
Makes sure we promote 'long', and the other integral types (char/bool) as well.
Attachment #459050 -
Attachment is obsolete: true
Attachment #459051 -
Attachment is obsolete: true
Attachment #459582 -
Flags: review?(benjamin)
Updated•14 years ago
|
Attachment #459582 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 23•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/46b4e429fce9
blocking2.0: beta3+ → ?
Whiteboard: fixed-in-tracemonkey
Comment 24•14 years ago
|
||
Also detectable on trunk for layout/generic/test/test_backspace_delete.xul on x86_64-linux. +1 for pushing this to Fx4.
Updated•14 years ago
|
blocking2.0: ? → betaN+
Comment 25•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/46b4e429fce9
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•