test_jsctypes.js will crash on Windows x64 since ffi_call_win64 overrun memory

RESOLVED FIXED

Status

()

Core
js-ctypes
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: m_kato, Assigned: dwitte@gmail.com)

Tracking

Trunk
x86_64
Windows Vista
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 4 obsolete attachments)

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
Created attachment 444375 [details] [diff] [review]
patch
Comment on attachment 444375 [details] [diff] [review]
patch

Dan, should I fix libffi instead of?
Attachment #444375 - Flags: review?(dwitte)
(Assignee)

Comment 3

8 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.)
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
(Assignee)

Comment 6

8 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.
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

8 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.
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)
Created attachment 447979 [details] [diff] [review]
patch v2
Attachment #444375 - Attachment is obsolete: true
Attachment #447979 - Flags: review?(dwitte)
(Assignee)

Comment 11

8 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

Updated

8 years ago
Duplicate of this bug: 578064
(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

8 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)

Updated

8 years ago
Duplicate of this bug: 579127
(Assignee)

Comment 16

8 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

8 years ago
Created attachment 459050 [details] [diff] [review]
part 1: fix ffi_call usage
Assignee: m_kato → dwitte
Attachment #447979 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #459050 - Flags: review?(benjamin)
(Assignee)

Comment 18

8 years ago
Created attachment 459051 [details] [diff] [review]
part 2: fix ffi_closure usage
Attachment #459051 - Flags: review?(benjamin)
(Assignee)

Comment 19

8 years ago
Want this for Fx4 -- overrunning memory is bad.
blocking2.0: --- → ?

Comment 20

8 years ago
Indeed, sync is already crashing on Linux64 because of this.
blocking2.0: ? → beta3+

Updated

8 years ago
Attachment #459050 - Flags: review?(benjamin) → review+

Comment 21

8 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

8 years ago
Created attachment 459582 [details] [diff] [review]
patch v2

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

8 years ago
Attachment #459582 - Flags: review?(benjamin) → review+
(Assignee)

Comment 23

8 years ago
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.

Updated

8 years ago
blocking2.0: ? → betaN+

Comment 25

7 years ago
http://hg.mozilla.org/mozilla-central/rev/46b4e429fce9
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.