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)

x86_64
Windows Vista
defect
Not set
normal

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)

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.
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
Attached patch patch (obsolete) — Splinter Review
Comment on attachment 444375 [details] [diff] [review]
patch

Dan, should I fix libffi instead of?
Attachment #444375 - Flags: review?(dwitte)
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.)
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.
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
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.
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.
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.
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)
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #444375 - Attachment is obsolete: true
Attachment #447979 - Flags: review?(dwitte)
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
(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.
(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?
Comment on attachment 447979 [details] [diff] [review]
patch v2

r- since this doesn't solve the entire problem.
Attachment #447979 - Flags: review?(dwitte) → review-
Attached patch part 1: fix ffi_call usage (obsolete) — Splinter Review
Assignee: m_kato → dwitte
Attachment #447979 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #459050 - Flags: review?(benjamin)
Attached patch part 2: fix ffi_closure usage (obsolete) — Splinter Review
Attachment #459051 - Flags: review?(benjamin)
Want this for Fx4 -- overrunning memory is bad.
blocking2.0: --- → ?
Indeed, sync is already crashing on Linux64 because of this.
blocking2.0: ? → beta3+
Attachment #459050 - Flags: review?(benjamin) → review+
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-
Attached patch patch v2Splinter Review
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)
Attachment #459582 - Flags: review?(benjamin) → review+
http://hg.mozilla.org/tracemonkey/rev/46b4e429fce9
blocking2.0: beta3+ → ?
Whiteboard: fixed-in-tracemonkey
Also detectable on trunk for layout/generic/test/test_backspace_delete.xul
on x86_64-linux.  +1 for pushing this to Fx4.
blocking2.0: ? → betaN+
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.

Attachment

General

Created:
Updated:
Size: