1464789 - wasm LazyStubTier::createMany executableCopy segment fault

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[qiaopengcheng]

User Agent: Mozilla/5.0 (X11; Linux mips64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20180523145036

Steps to reproduce:

If the computer's memory page is not 4k-page,  the test-case import-export.js of the jit-test will trigger a segment fault.
cd jit-test
./jit-test --tbpl ../release_OPT.OBJ/dist/bin/js  import-export.js


Actual results:

If the computer's memory page is not 4k-page,  the test-case import-export.js of the jit-test will trigger a segment fault.


Expected results:

./jit-test --tbpl ../release_OPT.OBJ/dist/bin/js  import-export.js
the result should pass all.

Comment 1

[qiaopengcheng]

Attachment: if memory page is not 4k LazyStubTier::createMany executableCopy segment fault

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8981107 [details] [diff] [review]
if memory page is not 4k LazyStubTier::createMany executableCopy  segment fault

Forwarding to Luke, because Wasm.

Comment 3

[Jan de Mooij [:jandem]]

Looking at the code a bit, this is probably because MPROTECT_PAGE_SIZE [0] is 4 KB?

If that's true, we should probably fix that instead.

[0] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/js/src/wasm/WasmCode.h#510

Comment 4

[qiaopengcheng]

If the computer's memory page is not 4k-page,  the test-case import-export.js of the jit-test will trigger a segment fault.

When the computer's memory page is not 4k-page, for example 16K page-size, 
these codes in file wasm/WasmCode.cpp

 694     if (!segment->addStubs(codeLength, funcExportIndices, funcExports, codeRanges, &codePtr,
 695                            &interpRangeIndex))
 696         return false;
 697 
 698     masm.executableCopy(codePtr, /* flushICache = */ false);
 699     memset(codePtr + masm.bytesNeeded(), 0, codeLength - masm.bytesNeeded());
 700 
 701     ExecutableAllocator::cacheFlush(codePtr, codeLength);
 702     if (!ExecutableAllocator::makeExecutable(codePtr, codeLength))
 703         return false;


The first time entrance of LazyStubTier::createMany would make the memory page flag as excuteable, for example the codeLength is 4k is less than the os-sytem's page-size.

when the second time entrance of LazyStubTier::createMany, masm.executableCopy(codePtr, /* flushICache = */ false);  would write data-code the page but now the page flag is not writeable!
the js-engine would report a segment fault.

Comment 5

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Looking at the code a bit, this is probably because MPROTECT_PAGE_SIZE [0]
> is 4 KB?
> 
> If that's true, we should probably fix that instead.
> 
> [0]
> https://searchfox.org/mozilla-central/rev/
> 5a744713370ec47969595e369fd5125f123e6d24/js/src/wasm/WasmCode.h#510

Ok, I'll have a try.

Comment 6

[Jan de Mooij [:jandem]]

Maybe you can use gc::SystemPageSize() instead of that constant, or as a quick hack just hard-code your page size to see if it fixes this.

Comment 7

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #6)
> Maybe you can use gc::SystemPageSize() instead of that constant, or as a
> quick hack just hard-code your page size to see if it fixes this.

yes, I have test using gc::SystemPageSize() is OK

Comment 8

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #6)
> Maybe you can use gc::SystemPageSize() instead of that constant, or as a
> quick hack just hard-code your page size to see if it fixes this.

But I just using the 16*1024  instead of static constexpr size_t MPROTECT_PAGE_SIZE = gc::SystemPageSize(); for compiling error.

Comment 9

[qiaopengcheng]

Attachment: Bug-1464789-Modify-the-wasm-MPROTECT_PAGE_SIZE.patch

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8981135 [details] [diff] [review]
Bug-1464789-Modify-the-wasm-MPROTECT_PAGE_SIZE.patch

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

D'oh!  I don't know why I missed this in the initial review; of course we need to use the system page size.  I appreciate you minimizing the patch size, but we generally try to avoid macros; could you change the patch to remove MPROTECT_PAGE_SIZE and use gc::SystemPageSize() in AlignBytesNeeded()?  For the other use in LazyStubSegment::hasSpace(), I think what the assert really wants to say is:
  MOZ_ASSERT(AlignBytesNeeded(bytes) == bytes);
so could you change it to that?  I think those are the only two uses.  Thanks!

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8981107 [details] [diff] [review]
if memory page is not 4k LazyStubTier::createMany executableCopy  segment fault

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

With the other fix--using the system page size--I would think this patch wouldn't be necessary; is that right?

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 8981107 [details] [diff] [review]
if memory page is not 4k LazyStubTier::createMany executableCopy  segment fault

(Canceling review for now, but please re-request if the patch is indeed necessary.)

Comment 13

[qiaopengcheng]

Attachment: memory page is not 4k-page trigger segment fault

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 8981288 [details] [diff] [review]
memory page is not 4k-page trigger segment fault

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

Perfect, thanks!

Comment 15

[qiaopengcheng]

(In reply to Luke Wagner [:luke] from comment #14)
> Comment on attachment 8981288 [details] [diff] [review]
> memory page is not 4k-page trigger segment fault
> 
> Review of attachment 8981288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Perfect, thanks!

Thank a lot ！

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Can you please mark the obsolete patches as such? It's very difficult to tell which patches are currently supposed to be landing here. I assume it's just the one that Luke r+ed, but it's pretty unclear.

Comment 17

[qiaopengcheng]

Attachment: Obsoletes the old patch

Comment 18

[qiaopengcheng]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Can you please mark the obsolete patches as such? It's very difficult to
> tell which patches are currently supposed to be landing here. I assume it's
> just the one that Luke r+ed, but it's pretty unclear.


I don't know how to Obsoletes the old patch after having updating the newest patch.
Is this ok ?

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4c476186ab
If page-size is not 4K, the function of LazyStubTier::createMany would trigger a segment fault within executableCopy. r=luke

Comment 20

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/fc4c476186ab