Closed Bug 1464789 Opened 5 years ago Closed 5 years ago

wasm LazyStubTier::createMany executableCopy segment fault

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Version:
62 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: qiaopengcheng-hf, Assigned: qiaopengcheng-hf)

Details

Attachments

(2 files, 2 obsolete files)

memory page is not 4k-page trigger segment fault
1.83 KB, patch
luke
: review+
Details | Diff | Splinter Review
Obsoletes the old patch
23 bytes, text/plain
Details 
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.
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Comment on attachment 8981107 [details] [diff] [review]
if memory page is not 4k LazyStubTier::createMany executableCopy  segment fault

Forwarding to Luke, because Wasm.
Attachment #8981107 - Flags: review?(jdemooij) → review?(luke)
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
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.
(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.
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.
(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
(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.

        Attachment #8981135 -
        Flags: review?(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!

        Attachment #8981135 -
        Flags: review?(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 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.)

        Attachment #8981107 -
        Flags: review?(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!

        Attachment #8981288 -
        Flags: review?(luke) → review+

    
    

    
  
(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 ！

    
  
Keywords: checkin-needed
Version: 61 Branch → 62 Branch
Assignee: nobody → qiaopengcheng-hf

    
    

    
  
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(qiaopengcheng-hf)
Keywords: checkin-needed

    
  
Flags: needinfo?(qiaopengcheng-hf)

    
    

    
  


  

        Attachment #8981107 -
        Attachment is obsolete: true

        Attachment #8981135 -
        Attachment is obsolete: true

    
    

    
  
(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 ?

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by rgurzau@mozilla.com:
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
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/fc4c476186ab
Status: ASSIGNED → RESOLVED
Closed: 5 years ago

          status-firefox62:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

          You need to log in
          before you can comment on or make changes to this bug.