Closed Bug 1020517 Opened 5 years ago Closed 5 years ago

Assertion failure: (munmap(code, totalBytes) == 0), at jit/AsmJSModule.cpp

Categories

(Core :: JavaScript: GC, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox32 --- affected

People

(Reporter: gkw, Assigned: sunfish)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(2 files)

Attached file stack
+++ This bug was initially created as a clone of Bug #1008613 +++

The upcoming testcase asserts js debug shell on m-c changeset c7fdd7e755cd with --no-baseline --ion-parallel-compile=off --ion-eager at Assertion failure: (munmap(code, totalBytes) == 0), at jit/AsmJSModule.cpp

Sometimes this needs ASLR disabled, sometimes this doesn't, to make this more reliable. See http://stackoverflow.com/a/5194709 for how to do so.

Setting s-s and sec-critical because this has been a pain to reduce, is intermittent, and is a gc-related assertion. (Also cc'ing our gc gurus)

My configure flags are:

AR=ar sh /home/fuzz2lin/trees/mozilla-central/js/src/configure --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

Might be related to bug 1008613 but the stack looks a little different. s-s until this is confirmed to not be dangerous.
Setting needinfo? from Luke and/or Terrence, not sure if this is asm.js or gc.

I'll try to get a bisect, but not sure if it will be reliable.
Flags: needinfo?(terrence)
Flags: needinfo?(luke)
IIUC, this is just munmap failing.  Given that munmap is a fallible API, the assertion is technically invalid and could be removed.  We'd leak the memory, sure, but there's not much you can do when "free" fails.  I like the assertion, though, because it asserts that the API is being called correctly (in the 99.999% case where it doesn't fail because of some OOM situation).
Flags: needinfo?(luke)
When munmap fails with EINVAL, it's usually due to a bug, so an assertion for that is valid. According to bug 1008613, munmap can also return ENOMEM in some cases, possibly including the failure in this bug. If so, perhaps the assertion could be loosened to this:

    JS_ALWAYS_TRUE(munmap(code, totalBytes) == 0 || errno == ENOMEM);
Right I'm assuming it's ENOMEM failure here, but you're right we can loosen, but still keep, the assert.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/fe2e4e1be13c
user:        Sean Stangl
date:        Thu Feb 20 14:43:03 2014 -0800
summary:     Bug 933001 - Part 1/5 - Define SharedArrayBufferObject. r=sfink

Again, this may not necessarily be the cause. If this bug is just an OOM and not s-s, please feel free to open it up.
Flags: needinfo?(terrence)
Gary: does replacing the failing line in AsmJSModule.cpp with what Dan suggested in comment 4 fix the assert?  (If not, could you printf the errno?)
Getting this on my plate.
Flags: needinfo?(gary)
Compile failure:

In file included from /home/fuzz2lin/Desktop/shell-cache/js-dbg-64-dm-ts-linux-4fc2737aac94/objdir-js-dbg-4fc2737aac94-Um5iX8/js/src/Unified_cpp_js_src2.cpp:93:0:
/home/fuzz2lin/trees/mozilla-central/js/src/jit/AsmJSModule.cpp: In function ‘void DeallocateExecutableMemory(uint8_t*, size_t)’:
/home/fuzz2lin/trees/mozilla-central/js/src/jit/AsmJSModule.cpp:100:98: error: ‘errno’ was not declared in this scope
     JS_ALWAYS_TRUE(munmap(code, totalBytes) == 0 || errno == ENOMEM);
                                                                                                  ^
/home/fuzz2lin/trees/mozilla-central/js/src/jit/AsmJSModule.cpp:100:107: error: ‘ENOMEM’ was not declared in this scope
     JS_ALWAYS_TRUE(munmap(code, totalBytes) == 0 || errno == ENOMEM);
                                                                                                           ^
/home/fuzz2lin/trees/mozilla-central/js/src/jit/AsmJSModule.cpp:100:117: error: no matching function for call to ‘ValidateAssertConditionType()’
     JS_ALWAYS_TRUE(munmap(code, totalBytes) == 0 || errno == ENOMEM);
                                                                                                                     ^
/home/fuzz2lin/trees/mozilla-central/js/src/jit/AsmJSModule.cpp:100:117: note: candidate is:
In file included from ../../dist/include/mozilla/ArrayUtils.h:14:0,
                 from ../../dist/include/mozilla/Vector.h:14,
                 from ../../dist/include/js/Vector.h:10,
                 from /home/fuzz2lin/trees/mozilla-central/js/src/jscntxt.h:14,
                 from /home/fuzz2lin/trees/mozilla-central/js/src/irregexp/RegExpEngine.h:34,
                 from /home/fuzz2lin/trees/mozilla-central/js/src/irregexp/RegExpAST.h:34,
                 from /home/fuzz2lin/trees/mozilla-central/js/src/irregexp/RegExpMacroAssembler.h:34,
                 from /home/fuzz2lin/trees/mozilla-central/js/src/irregexp/RegExpInterpreter.cpp:34,
                 from /home/fuzz2lin/Desktop/shell-cache/js-dbg-64-dm-ts-linux-4fc2737aac94/objdir-js-dbg-4fc2737aac94-Um5iX8/js/src/Unified_cpp_js_src2.cpp:2:
../../dist/include/mozilla/Assertions.h:334:6: note: template<class T> void mozilla::detail::ValidateAssertConditionType()
 void ValidateAssertConditionType()
      ^
../../dist/include/mozilla/Assertions.h:334:6: note:   template argument deduction/substitution failed:
In file included from /home/fuzz2lin/Desktop/shell-cache/js-dbg-64-dm-ts-linux-4fc2737aac94/objdir-js-dbg-4fc2737aac94-Um5iX8/js/src/Unified_cpp_js_src2.cpp:93:0:
/home/fuzz2lin/trees/mozilla-central/js/src/jit/AsmJSModule.cpp:100:117: error: template argument 1 is invalid
     JS_ALWAYS_TRUE(munmap(code, totalBytes) == 0 || errno == ENOMEM);
                                                                                                                     ^
Flags: needinfo?(gary) → needinfo?(sunfish)
Oh, you need to #include "errno.h".
Flags: needinfo?(sunfish)
Attached patch patchSplinter Review
Since Dan suggested this patch, I've put in his name.
Attachment #8435871 - Flags: review?(luke)
I tested this patch with the following results:

Without the patch, the testcase asserts when run once every 5-10 times.
With the patch, the testcase didn't assert when I ran it 90 times.
This means that munmap is failing due to OOM, removing sec flags.
Group: core-security, javascript-core-security
Comment on attachment 8435871 [details] [diff] [review]
patch

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

Great, thanks!

::: js/src/jit/AsmJSModule.cpp
@@ +27,5 @@
>  
>  #include "jsobjinlines.h"
>  
>  #include "frontend/ParseNode-inl.h"
> +#include "errno.h"

I told you wrong, it should be #include <errno.h>.  SM include style
  https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style#.23include_ordering
will want you to put this #include after the mozilla/ #includes and before the js*.h.  I'd run 'make check-style' to be sure.
Attachment #8435871 - Flags: review?(luke) → review+
I spoke to Dan about this over IRC and he agreed to take this to completion.
Assignee: nobody → sunfish
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/b0561d529f95
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.