Closed Bug 1499277 Opened Last year Closed Last year

StructuredClone endianness fixes are incomplete

Categories

(Core :: JavaScript Engine, defect, P3, major)

64 Branch
Desktop
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: awilfox, Assigned: awilfox)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1481740 ( https://hg.mozilla.org/mozilla-central/rev/c7d62fc647ec ) removed endianness swapping for pointers.  Bug 1485460 ( https://hg.mozilla.org/mozilla-central/rev/b23f0be9191b ) put them back, but did not reapply the endianness swap in readPtr.  This bustage was giving me crashes in LZ4 when I tried to |mach run| the browser:

#3  0x000000011542a7bc in mozilla::Compression::LZ4::compress(char const*, unsigned long, char*) (aSource=0x4004855601000000 <error: Cannot access memory at address 0x4004855601000000>, aInputSize=<optimized out>, aDest=0x156a8f9ec "")
    at /var/clean/mozppc/mozilla-unified/mfbt/Compression.cpp:26

Note how, if you byteswap aSource, it becomes 0x156850440:

(gdb) p (char const *)0x0156850440
$4 = 0x156850440 "{\"version\":1,\"buildID\":\"20181013160528\",\"appVersion\":\"64.0a1\",\"locale\":\"en-US\",\"visibleDefaultEngines\":[\"amazondotcom\",\"bing\",\"ebay\",\"google-2018\",\"twitter\",\"wikipedia\",\"ddg\"],\"metaData\":{\"searchDefau"...

This also showed up in jsapi-tests, which made it thankfully much easier to poke at:

testMappedArrayBuffer_bug945152

Thread 1 "jsapi-tests" hit Breakpoint 1, 0x000000010086ad48 in JS_GetArrayBufferData (obj=0x3ffff4aa9080, isSharedMemory=0x3fffffffecf7) at /var/clean/mozppc/mozilla-unified/js/src/vm/ArrayBufferObject.cpp:1835
1835    {
(gdb) bt
#0  0x000000010086ad48 in JS_GetArrayBufferData (obj=0x3ffff4aa9080, isSharedMemory=0x3fffffffecf7) at /var/clean/mozppc/mozilla-unified/js/src/vm/ArrayBufferObject.cpp:1835
#1  0x00000001002f3768 in cls_testMappedArrayBuffer_bug945152::VerifyObject (this=0x1016a49d8 <cls_testMappedArrayBuffer_bug945152_instance>, obj=..., offset=<optimized out>, length=<optimized out>, mapped=<optimized out>)
    at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/testMappedArrayBuffer.cpp:96
#2  0x00000001002f3a48 in cls_testMappedArrayBuffer_bug945152::TestCreateObject (this=0x1016a49d8 <cls_testMappedArrayBuffer_bug945152_instance>, offset=<optimized out>, length=<optimized out>)
    at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/testMappedArrayBuffer.cpp:106
#3  0x00000001002fe198 in cls_testMappedArrayBuffer_bug945152::run (this=0x1016a49d8 <cls_testMappedArrayBuffer_bug945152_instance>, global=...) at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/testMappedArrayBuffer.cpp:35
#4  0x0000000100256de0 in main (argc=<optimized out>, argv=<optimized out>) at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/tests.cpp:134
(gdb) finish
cls_testMappedArrayBuffer_bug945152::VerifyObject (this=0x1016a49d8 <cls_testMappedArrayBuffer_bug945152_instance>, obj=..., offset=<optimized out>, length=<optimized out>, mapped=<optimized out>)
    at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/testMappedArrayBuffer.cpp:97
97          CHECK(data);
(gdb) p data
$1 = 0x3ffff7ff9008 "90ABCDEFGHIJ"
(gdb) c
Continuing.

Thread 1 "jsapi-tests" hit Breakpoint 1, 0x000000010086ad48 in JS_GetArrayBufferData (obj=0x3ffff4aa40a0, isSharedMemory=0x3fffffffebd7) at /var/clean/mozppc/mozilla-unified/js/src/vm/ArrayBufferObject.cpp:1835
1835    {
(gdb) bt
#0  0x000000010086ad48 in JS_GetArrayBufferData (obj=0x3ffff4aa40a0, isSharedMemory=0x3fffffffebd7) at /var/clean/mozppc/mozilla-unified/js/src/vm/ArrayBufferObject.cpp:1835
#1  0x00000001002f3768 in cls_testMappedArrayBuffer_bug945152::VerifyObject (this=0x1016a49d8 <cls_testMappedArrayBuffer_bug945152_instance>, obj=..., offset=<optimized out>, length=<optimized out>, mapped=<optimized out>)
    at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/testMappedArrayBuffer.cpp:96
#2  0x00000001002f8c40 in cls_testMappedArrayBuffer_bug945152::TestCloneObject (this=0x1016a49d8 <cls_testMappedArrayBuffer_bug945152_instance>) at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/testMappedArrayBuffer.cpp:144
#3  0x00000001002fe448 in cls_testMappedArrayBuffer_bug945152::run (this=0x1016a49d8 <cls_testMappedArrayBuffer_bug945152_instance>, global=...) at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/testMappedArrayBuffer.cpp:50
#4  0x0000000100256de0 in main (argc=<optimized out>, argv=<optimized out>) at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/tests.cpp:134
(gdb) finish
cls_testMappedArrayBuffer_bug945152::VerifyObject (this=0x1016a49d8 <cls_testMappedArrayBuffer_bug945152_instance>, obj=..., offset=<optimized out>, length=<optimized out>, mapped=<optimized out>)
    at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/testMappedArrayBuffer.cpp:97
97          CHECK(data);
(gdb) p data
$2 = 0x3ffff4aa40e0 "90ABCDEFGHIJMMMM\377\376MMMMMM\377\376MMMMMM"
(gdb) c
Continuing.

Thread 1 "jsapi-tests" hit Breakpoint 1, 0x000000010086ad48 in JS_GetArrayBufferData (obj=0x3ffff4aa91c0, isSharedMemory=0x3fffffffeaf7) at /var/clean/mozppc/mozilla-unified/js/src/vm/ArrayBufferObject.cpp:1835
1835    {
(gdb) bt
#0  0x000000010086ad48 in JS_GetArrayBufferData (obj=0x3ffff4aa91c0, isSharedMemory=0x3fffffffeaf7) at /var/clean/mozppc/mozilla-unified/js/src/vm/ArrayBufferObject.cpp:1835
#1  0x00000001002f3768 in cls_testMappedArrayBuffer_bug945152::VerifyObject (this=0x1016a49d8 <cls_testMappedArrayBuffer_bug945152_instance>, obj=..., offset=<optimized out>, length=<optimized out>, mapped=<optimized out>)
    at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/testMappedArrayBuffer.cpp:96
#2  0x00000001002fdf4c in cls_testMappedArrayBuffer_bug945152::TestTransferObject (this=0x1016a49d8 <cls_testMappedArrayBuffer_bug945152_instance>) at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/testMappedArrayBuffer.cpp:182
#3  0x00000001002fe470 in cls_testMappedArrayBuffer_bug945152::run (this=0x1016a49d8 <cls_testMappedArrayBuffer_bug945152_instance>, global=...) at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/testMappedArrayBuffer.cpp:56
#4  0x0000000100256de0 in main (argc=<optimized out>, argv=<optimized out>) at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/tests.cpp:134
(gdb) finish
cls_testMappedArrayBuffer_bug945152::VerifyObject (this=0x1016a49d8 <cls_testMappedArrayBuffer_bug945152_instance>, obj=..., offset=<optimized out>, length=<optimized out>, mapped=<optimized out>)
    at /var/clean/mozppc/mozilla-unified/js/src/jsapi-tests/testMappedArrayBuffer.cpp:97
97          CHECK(data);
(gdb) p data
$3 = 0x860fff7ff3f0000 <error: Cannot access memory at address 0x860fff7ff3f0000>
(gdb)
Attached patch adds the check back to readPtr, which allows jsapi-tests to pass successfully - and the engine now runs flawlessly on 64-bit big endian PowerPC!
Attachment #9017404 - Flags: review?(sphink)
Comment on attachment 9017404 [details] [diff] [review]
Fix SCInput::readPtr on big endian systems.

Review of attachment 9017404 [details] [diff] [review]:
-----------------------------------------------------------------

Wow. I can't believe I missed this over in bug 1485460. Thanks!
Attachment #9017404 - Flags: review?(sphink) → review+
I may be confused, but it seems like we should remove SCInput::readNativeEndian entirely and use SCInput::read() here.
Flags: needinfo?(sphink)
(In reply to Jason Orendorff [:jorendorff] from comment #3)
> I may be confused, but it seems like we should remove
> SCInput::readNativeEndian entirely and use SCInput::read() here.

I do not think you are confused. I think you are right.

readNativeEndian is rather pointless now that we're no longer storing anything in native endian format. We have as well have SCInput::readEBCDIC.
Flags: needinfo?(sphink)
New revision, tweaking readPtr and removing the superfluous readNativeEndian.
Attachment #9017404 - Attachment is obsolete: true
Attachment #9017722 - Flags: review?(sphink)
Attachment #9017722 - Flags: review?(sphink) → review+
Assignee: nobody → AWilcox
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(jorendorff)
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a58f94c3f5a
Remove unnecessary SCInput::readNativeEndian; fix SCInput::readPtr on big endian systems. r=sfink
Keywords: checkin-needed
Flags: needinfo?(jorendorff)
https://hg.mozilla.org/mozilla-central/rev/3a58f94c3f5a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.