Closed Bug 1229855 Opened 9 years ago Closed 9 years ago

Crash [@ js::wasm::ToMIRType] or Crash on Heap

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- wontfix
firefox46 --- verified
firefox-esr38 --- wontfix
firefox-esr45 47+ fixed

People

(Reporter: decoder, Unassigned)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [fuzzblocker] [jsbugmon:][adv-main46+])

Crash Data

Attachments

(1 file)

The following testcase crashes on mozilla-central revision 470f4f8c2b2d (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2 --ion-offthread-compile=off): function g() { "use asm"; function f() { gczeal(2); } } Backtrace: Program received signal SIGSEGV, Segmentation fault. 0x08200c2a in js::wasm::ToMIRType (vt=js::wasm::I32) at js/src/asmjs/Wasm.h:56 #0 0x08200c2a in js::wasm::ToMIRType (vt=js::wasm::I32) at js/src/asmjs/Wasm.h:56 #1 0x08234f35 in FunctionCompiler::passArg (this=this@entry=0xffff9200, argDef=0xf7a9c588, type=js::wasm::I32, call=call@entry=0xffff8f70) at js/src/asmjs/WasmIonCompile.cpp:664 #2 0x082179cc in EmitCallArgs (f=..., sig=..., call=call@entry=0xffff8f70) at js/src/asmjs/WasmIonCompile.cpp:1542 #3 0x0821849e in EmitInternalCall (f=..., ret=ret@entry=js::wasm::Void, def=def@entry=0xffff9050) at js/src/asmjs/WasmIonCompile.cpp:1567 #4 0x0821454d in EmitStatement (f=..., stmt=<optimized out>, maybeLabels=0x0) at js/src/asmjs/WasmIonCompile.cpp:2497 #5 0x08214739 in EmitStatement (f=..., maybeLabels=0x0) at js/src/asmjs/WasmIonCompile.cpp:2515 #6 0x0821d2e5 in js::wasm::CompileFunction (task=0xf7a9b000) at js/src/asmjs/WasmIonCompile.cpp:2949 #7 0x081ccb88 in js::wasm::ModuleGenerator::finishFunc (this=0xffffa398, funcIndex=1, sig=..., generateTime=0, fg=0xffffa1e8) at js/src/asmjs/WasmGenerator.cpp:180 #8 0x081e4c15 in finish (generateTime=<optimized out>, sig=..., funcIndex=<optimized out>, this=0xffffa1e0) at js/src/asmjs/AsmJSValidate.cpp:2087 #9 CheckFunction (m=...) at js/src/asmjs/AsmJSValidate.cpp:6578 #10 CheckFunctions (m=...) at js/src/asmjs/AsmJSValidate.cpp:6609 #11 CheckModule (cx=cx@entry=0xf7a7b020, parser=..., stmtList=stmtList@entry=0xf7a880e8, module=module@entry=0xffffb1e0, time=time@entry=0xffffb1dc, slowFuncs=slowFuncs@entry=0xffffb220) at js/src/asmjs/AsmJSValidate.cpp:6829 #12 0x081eaff3 in js::ValidateAsmJS (cx=0xf7a7b020, parser=..., stmtList=stmtList@entry=0xf7a880e8, validated=validated@entry=0xffffb270) at js/src/asmjs/AsmJSValidate.cpp:6992 #13 0x08130195 in js::frontend::Parser<js::frontend::FullParseHandler>::asmJS (this=0xffffc0d8, list=0xf7a880e8) at js/src/frontend/Parser.cpp:3184 [...] #35 main (argc=5, argv=0xffffce24, envp=0xffffce3c) at js/src/shell/js.cpp:6874 eax 0x4d430000 1296236544 ebx 0x9822970 159525232 ecx 0x0 0 edx 0x0 0 esi 0xffff8f70 -28816 edi 0xffff9200 -28160 ebp 0xffff8e78 4294938232 esp 0xffff8e60 4294938208 eip 0x8200c2a <js::wasm::ToMIRType(js::wasm::ValType)+26> => 0x8200c2a <js::wasm::ToMIRType(js::wasm::ValType)+26>: mov -0x1621d38(%ebx,%eax,4),%eax 0x8200c31 <js::wasm::ToMIRType(js::wasm::ValType)+33>: mov %ebx,%ecx
Summary: Crash [@ js::wasm::ToMIRType] → Crash [@ js::wasm::ToMIRType] or Crash on Heap
Flags: needinfo?(benj)
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [fuzzblocker] [jsbugmon:bisect] → [fuzzblocker] [jsbugmon:]
I'm unable to repro with the given build/run-time flags at the given cset. I'm testing on Ubuntu with gcc. Does the crash still repro on tip? Also, just on a hunch, does it go away by changing the MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE in Wasm.h:64 to to MOZ_CRASH?
Could reproduce this and bug 1228773 with a chrooted Ubuntu 14.04, using gcc 4.8.2 and the following build.sh file: CC="gcc -m32" CXX="g++ -m32" AR="ar" $repo/js/src/configure --without-intl-api --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-debug This one doesn't reproduce without --enable-valgrind. The other one even reproduces without --enable-valgrind. Investigating.
Flags: needinfo?(benj)
Great job Benjamin, and thanks for the help. I'm guessing this is actually my bug, but if you're able to help track it down, that'd be fantastic; let me know if I can help.
Attached patch duh.patch — — Splinter Review
Duh. Very much fortunately, I've been working on bug 1229338 recently, where I've used an enum class as well, and ehoogeven pointed to a weird gcc compilation bug (see https://bugzilla.mozilla.org/show_bug.cgi?id=1229338#c3 which redirects to bug 1143966). See how the fix is similar? I've tried this, out of luck and as it really sounded like the same kind of situation (compiler bug using enum class with underlying storage being uint8_t), and it seems to solve the issue locally, plus the test case in bug 1228773. So a big thank you Emmanuel for sparing a lot of headaches to all of us! Christian, if you apply this patch, does it fix the issue or other similar test cases? (this bug is marked as a fuzz-blocker, so i suppose you do have other test cases) Alon, ditto for bug 1228773?
Attachment #8695970 - Flags: review?(luke)
Attachment #8695970 - Flags: feedback?(choller)
Attachment #8695970 - Flags: feedback?
Comment on attachment 8695970 [details] [diff] [review] duh.patch Review of attachment 8695970 [details] [diff] [review]: ----------------------------------------------------------------- Holy crap, that's terrible!
Attachment #8695970 - Flags: review?(luke) → review+
Ehsan: just asking as the representative of "using new C++ features in Mozilla": is this a known bug? Could we either bump our oldest-supported compiler version or perhaps this is something we need to rule out via clang static analysis?
Flags: needinfo?(ehsan)
Btw, Benjamin, thanks! By any chance is the bug restricted to uint8? If so and we weren't able to bump our minimum gcc version, then perhaps that is what the clang static analysis would reject.
(In reply to Luke Wagner [:luke] from comment #7) > Ehsan: just asking as the representative of "using new C++ features in > Mozilla": is this a known bug? First I'm hearing about it! > Could we either bump our oldest-supported > compiler version or perhaps this is something we need to rule out via clang > static analysis? Upgrading the minimum gcc version is complicated as it's usually gated on old b2g branches. We can definitely disallow it with static analysis. Is it just uint8_t as the storage type? Also, do we know which version of gcc has fixed this?
Flags: needinfo?(ehsan)
Ehsan: if I'm reading bug 1143966 comment 10 and https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64037 correctly, it seems like this was a regression introduced in 4.8/4.9 and fixed in 4.8.5 and 4.9.3. So perhaps a fix could be to not bump the minimum version but rather just disallow [4.8.0-4.8.5]? Benjamin: from a quick scan, it looks like the bug is restricted to byte types when used as non-inlined return values. (It sounds like another incarnation of the old "can I return trash in the upper 3 bytes" ABI issue.) If you put MOZ_ALWAYS_INLINE on ToMIRType, does it fix the crash? That might be a nicer, more-targeted fix. Also, if we can't disallow the broken GCC versions, then then the clang check could only disallow enum with sub-word rep type on non-MOZ_ALWAYS_INLINE function...
(In reply to Luke Wagner [:luke] from comment #10) > Ehsan: if I'm reading bug 1143966 comment 10 and > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64037 correctly, it seems like > this was a regression introduced in 4.8/4.9 and fixed in 4.8.5 and 4.9.3. > So perhaps a fix could be to not bump the minimum version but rather just > disallow [4.8.0-4.8.5]? Are you sure? I'm 99% sure that we use 4.7.3 with some patches on production. <https://dxr.mozilla.org/mozilla-central/source/build/unix/build-gcc/build-gcc.sh#3>
(In reply to Luke Wagner [:luke] from comment #8) > Btw, Benjamin, thanks! By any chance is the bug restricted to uint8? If so > and we weren't able to bump our minimum gcc version, then perhaps that is > what the clang static analysis would reject. This also reproduces with uint16. Haven't tested with uint32, as this is default on x86. (In reply to Luke Wagner [:luke] from comment #10) > Benjamin: from a quick scan, it looks like the bug is restricted to byte > types when used as non-inlined return values. (It sounds like another > incarnation of the old "can I return trash in the upper 3 bytes" ABI issue.) > If you put MOZ_ALWAYS_INLINE on ToMIRType, does it fix the crash? That > might be a nicer, more-targeted fix. Also, if we can't disallow the broken > GCC versions, then then the clang check could only disallow enum with > sub-word rep type on non-MOZ_ALWAYS_INLINE function... This is actually happening in ToMIRType, which is much likely to get inlined in these builds (in this bug and Alon's bug, this happens with --enable-optimize). Thus having MOZ_ALWAYS_INLINE doesn't fix the issue. Yet making them non static and MOZ_NEVER_INLINE and implementing them in a random cpp file fixes this bug, it doesn't fix Alon's issue. Luke said that we might want to disallow this compiler's version (gcc v4.8.2) in autoconf, in the meanwhile. For what it's worth, this version of gcc is e.g. in Ubuntu 14.04 but not in the next one, so it does sound outdated. The second best solution would be to have the static analysis forbid the use of enum classes with less-than-a-word wide storage as return values. Ehsan, what does sound the most appropriate/doable in your opinion?
Flags: needinfo?(ehsan)
If you ban the default compiler of Ubuntu 14.04 and RHEL 7 that will be tough. Keep in mind that 14.04 is the current LTS version of Ubuntu and will probably remain in use for quite a while. Many people will find it unreasonable if you can't build our software with the Ubuntu LTS version I guess. The nicest thing would be if they would backport this fix to 14.04.
Comment on attachment 8695970 [details] [diff] [review] duh.patch Confirmed, this patch fixes the crash for me. Can we land this to unblock fuzzing?
Attachment #8695970 - Flags: feedback?(choller) → feedback+
(In reply to Benjamin Bouvier [:bbouvier] from comment #12) > Luke said that we might want to disallow this compiler's version (gcc > v4.8.2) in autoconf, in the meanwhile. For what it's worth, this version of > gcc is e.g. in Ubuntu 14.04 but not in the next one, so it does sound > outdated. The second best solution would be to have the static analysis > forbid the use of enum classes with less-than-a-word wide storage as return > values. Ehsan, what does sound the most appropriate/doable in your opinion? I tried to write a static analysis that would catch this pattern but we seem to be using this pretty heavily. Given that, and the fact that the gcc versions in questions do not affect our binary releases, I'm inclined to say we should probably disallow the specific broken gcc versions in configure. But I'd like glandium to also weigh in.
Flags: needinfo?(ehsan) → needinfo?(mh+mozilla)
We've never autoconf'ed versions of gcc for its miscompilation bugs, afair. How often do we use this pattern, and would it make sense to macro it away on the problematic gcc versions? (That said, it might be better to do an actual detection of the bug with a miniature testcase than to rely on the gcc version)
Flags: needinfo?(mh+mozilla)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Group: javascript-core-security → core-security-release
Needinfo ehsan for comment 17.
Flags: needinfo?(ehsan)
Comment on attachment 8695970 [details] [diff] [review] duh.patch Sigh, I was pretty sure I set a needinfo for Alon to confirm this fixed his issue, but it got dismissed somehow.
Attachment #8695970 - Flags: feedback?
You might have needed to cc him on this s-s bug first. Alon: ref comment 5 and comment 20.
Flags: needinfo?(azakai)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #21) > You might have needed to cc him on this s-s bug first. > > Alon: ref comment 5 and comment 20. Alon confirmed in bug 1228773 that he couldn't see the issue on mozilla-central. Sorry for the noise.
Flags: needinfo?(azakai)
I also confirmed now that specifically the changeset from here is what fixed it.
(In reply to Benjamin Bouvier [:bbouvier] from comment #19) > Needinfo ehsan for comment 17. I'm the wrong person to provide a test case. I know nothing about the underlying gcc bug and how to trigger it. :-)
Flags: needinfo?(ehsan)
The needinfo was set for getting an answer to this question: (In reply to Mike Hommey [:glandium] (VAC: 31 Dec - 11 Jan) from comment #17) > How often do we use this pattern, and would it make sense to macro it away > on the problematic gcc versions?
Flags: needinfo?(ehsan)
We use the pattern of returning an enum type with an underlying type other than int several hundred times as I mentioned in comment 16. Since I don't know what is the exact case where gcc miscompiles, I can't say how many times that pattern is used. If we have exact criteria for what compilers miscompile this (aka a minimized test case) I'd much rather break those build rather than sprinkling macros all over our code.
Flags: needinfo?(ehsan)
JSBugMon: This bug has been automatically verified fixed on Fx46
Ehsan, Mike: for what it's worth, we havbe another instance of that in bug 1243810, where getting rid of the storage class associated to an enum class fixes a segfault that doesn't reproduce with recent versions of gcc.
Why was this checked in without a security rating or sec-approval+? This is a security bug that affected multiple releases. We need the sec-approval? template questions set on the patch so we can determine whether we need to take this on ESR45, which is affected by this issue, and to determine a security rating for this.
Whiteboard: [fuzzblocker] [jsbugmon:] → [fuzzblocker] [jsbugmon:][adv-main46+]
(In reply to Al Billings [:abillings] from comment #29) > Why was this checked in without a security rating or sec-approval+? This is > a security bug that affected multiple releases. > > We need the sec-approval? template questions set on the patch so we can > determine whether we need to take this on ESR45, which is affected by this > issue, and to determine a security rating for this. For what it's worth, the patch got checked in by :nbp, according to comment 15. But I think it was still a safe thing to do: the source of the bug is a compiler bug leading to miscompiled code, it happens only when we compile with non standard flags (--enable-valgrind), depending on the version of gcc you're using. It has been fixed in the latest versions of gcc; according to bug 1175546, we're now using gcc 4.8.5 and comment 10 says the bug ought to be fixed in this version of gcc. Overall, I still think it should be uplifted, if ESR is affected.
Flags: needinfo?(bbouvier)
Do we compile ESR with valgrind enabled? I don't think we do so if this only affects certain non-standard flags, why would we take this on ESR? For Tor or someone else?
Flags: needinfo?(bbouvier)
Whiteboard: [fuzzblocker] [jsbugmon:][adv-main46+] → [fuzzblocker] [jsbugmon:][adv-main46-]
Taking the patch would ensure that old compilers can still compile and run correctly asm.js, without crashing. Not even sure that Tor would enable asm.js in the first place (I seem to recall JITs are disabled in the most secure settings of Tor). Considering the size of the patch, it doesn't seem too risky to take; but I leave it to your judgement now.
Flags: needinfo?(bbouvier)
Whiteboard: [fuzzblocker] [jsbugmon:][adv-main46-] → [fuzzblocker] [jsbugmon:][adv-main46+]
Let's get this on the ESR-45 branch for the next release.
Another ESR45 bug that hasn't made it in for 45.2.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Can you request uplift to esr45 please?
Flags: needinfo?(lhenry) → needinfo?(nicolas.b.pierron)
Comment on attachment 8695970 [details] [diff] [review] duh.patch [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: See comment 30 and comment 33. User impact if declined: Impact all Ubuntu / RHEL users using asm.js, unless they updated the gcc version since comment 13. Fix Landed on Version: 46 Risk to taking this patch (and alternatives if risky): N/A String or UUID changes made by this patch: N/A See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(nicolas.b.pierron)
Attachment #8695970 - Flags: approval-mozilla-esr45?
Can you still do the sec-approval too? I know this already landed but the extra info is nice to have both for me and for if Al needs to write a sec advisory
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8695970 [details] [diff] [review] duh.patch Let's take this for ESR 45 since it affects some ESR users being able to compile.
Attachment #8695970 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Flags: needinfo?(nicolas.b.pierron)
Group: core-security-release
Blocks: 1228773
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: