Bug 138543 – Assertions in JSC::StackVisitor::Frame::existingArguments() during stack unwinding

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Akos Kiss]

When running jsc tests, exceptionFuzz/earley-boyer.js fails sporadically (experienced both on x86_64 and ARM64). The following 2 commands reproduce the assertions reliably, however:

WebKitBuild/Debug/bin/jsc --enableExceptionFuzz=true --fireExceptionFuzzAt=15006 Source/JavaScriptCore/tests/exceptionFuzz/earley-boyer.js
JSC EXCEPTION FUZZ: Throwing fuzz exception with call frame 0x7ffffe28dc00, seen in CommonSlowPaths and return address 0x11cc168.
ASSERTION FAILED: isCell()
../../Source/JavaScriptCore/runtime/JSCJSValueInlines.h(494) : JSC::JSCell* JSC::JSValue::asCell() const
1   0x7f7dd496754c /home/akiss/devel/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x1e) [0x7f7dd496754c]
2   0x42e53d WebKitBuild/Debug/bin/jsc(_ZNK3JSC7JSValue6asCellEv+0x3d) [0x42e53d]
3   0x42c4a1 WebKitBuild/Debug/bin/jsc(_ZN3JSC8asObjectENS_7JSValueE+0x18) [0x42c4a1]
4   0x7f7dd45c7d9c /home/akiss/devel/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC12asActivationENS_7JSValueE+0x21) [0x7f7dd45c7d9c]
5   0x7f7dd45c7e0e /home/akiss/devel/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZNK3JSC8Register18lexicalEnvironmentEv+0x20) [0x7f7dd45c7e0e]
6   0x7f7dd45c7635 /home/akiss/devel/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZNK3JSC9ExecState18lexicalEnvironmentEv+0x8f) [0x7f7dd45c7635]
7   0x7f7dd45d3d6a /home/akiss/devel/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC12StackVisitor5Frame17existingArgumentsEv+0xe8) [0x7f7dd45d3d6a]

WebKitBuild/Debug/bin/jsc --enableExceptionFuzz=true --fireExceptionFuzzAt=15009 Source/JavaScriptCore/tests/exceptionFuzz/earley-boyer.js
JSC EXCEPTION FUZZ: Throwing fuzz exception with call frame 0x7fff9e8dab50, seen in CommonSlowPaths and return address 0x1d959d0.
ASSERTION FAILED: from.isCell() && from.asCell()->JSCell::inherits(std::remove_pointer<To>::type::info())
../../Source/JavaScriptCore/runtime/JSCell.h(249) : To JSC::jsCast(JSC::JSValue) [with To = JSC::Arguments*]
1   0x7f662090057e /home/akiss/devel/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrashWithSecurityImplication+0x1e) [0x7f662090057e]
2   0x7f662056d918 /home/akiss/devel/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC6jsCastIPNS_9ArgumentsEEET_NS_7JSValueE+0x6a) [0x7f662056d918]
3   0x7f662056cd84 /home/akiss/devel/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC12StackVisitor5Frame17existingArgumentsEv+0x102) [0x7f662056cd84]

It turns out that ExceptionFuzz fires in a function containing an op_create_lexical_environment followed by an op_create_arguments, when either the lexicalEnvironment or the unmodifiedArgumentsRegister in the lexicalEnvironment is not set up yet. The function that ExceptionFuzz is firing at is:

BgL_nboyerzd2benchmarkzd2#CMhDdb:[0x1e8e610->0x7f50e98bad70, %sNoneFunctionCall, 223]: 223 m_instructions; 1784 bytes; 1 parameter(s); 24 callee register(s); 7 variable(s); 3 captured var(s) (from r-3 to r-5, inclusive); uses arguments, in r-4, r-3; lexical environment in r-1
[   0] enter             
[   1] get_scope         arg-3
[   3] create_lexical_environment loc0
[   5] init_lazy_reg     loc3
[   7] init_lazy_reg     loc2
[   9] create_arguments  loc3
[  11] put_to_scope      loc0, arguments(@id0), loc3, 3<ThrowIfNotFound|LocalClosureVar>, <structure>, -4
[  18] put_to_scope      loc0, arguments(@id0), loc2, 3<ThrowIfNotFound|LocalClosureVar>, <structure>, -3

And the lines in existingArguments() causing the assertions are:

    if (codeBlock()->needsActivation())
        return jsCast<Arguments*>(callFrame()->lexicalEnvironment()->registerAt(unmodifiedArgumentsRegister(reg).offset()).get());

If an exception is raised in op_enter or in op_get_scope then, although codeBlock()->needsActivation() is true, callFrame()->lexicalEnvironment() fails since the environment is undefined. If an exception is raised before the first put_to_scope then jsCast<Arguments*> fails since the unmodifiedArgumentsRegister of the lexicalEnvironment is undefined.

[Akos Kiss]

Created attachment 241252 [details]
Proposed patch.

The patch fixes both assertions and causes no jsc test regressions.

[Filip Pizlo]

Comment on attachment 241252 [details]
Proposed patch.

Can you write a test for this?

[Geoffrey Garen]

Comment on attachment 241252 [details]
Proposed patch.

When codeBlock->needsActivation() is true, how is it possible that the frame lacks an activation? I believe that should be impossible.

[Akos Kiss]

If I'm right, create_lexical_environment at [3] sets up activation to r-1. So, before that, although codeBlock()->needsActivation() is true, the frame does not have activation yet.

About the test: I'm not sure yet that I can force the first 6 instructions throw an exception... at least "naturally", but only from "outside" by ExceptionFuzz. When ExceptionFuzz was introduced in http://trac.webkit.org/changeset/171213 , StackVisitor was modified to handle throws in op_enter. I guess that was also only to cover such artificial fuzz exceptions. (At least I could not find any already existing test that would reliably trigger that exception, unfortunately.)

[Geoffrey Garen]

Looks like this has been failing on the bots periodically: <rdar://problem/18867723>.

[Geoffrey Garen]

I see: this failure is only possible in fuzzing mode, which sometimes inserts exceptions in places where they would otherwise be impossible.

[Geoffrey Garen]

Comment on attachment 241252 [details]
Proposed patch.

It feels a bit awkward to program defensively like this just to make the fuzzer happy. Programming like this means that we can't tell the difference between "Something is seriously wrong because the activation object is missing" and "I'm just fuzzing".

Ideally, we would teach the fuzzer not to throw in cases that otherwise couldn't -- for example, in the LLInt, by passing an argument to END() that said "ASSERT there is no exception, and do not fuzz for exceptions".

I guess this patch is an improvement for now, so it's worth doing. Note, though that you missed a spot: Oliver removed the original work-around for fuzzing, probably because he wasn't aware of this special fuzzing behavior.

You should update your comments to specify that we do this only for fuzzing, and also add back the code that Oliver removed in <http://trac.webkit.org/changeset/174226>:

Index: /trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp
===================================================================
--- /trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp	(revision 174225)
+++ /trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp	(revision 174226)
@@ -440,6 +440,4 @@
     CallFrame* callFrame = visitor->callFrame();
     CodeBlock* codeBlock = visitor->codeBlock();
-    JSScope* scope = callFrame->scope();
-
     if (Debugger* debugger = callFrame->vmEntryGlobalObject()->debugger()) {
         ClearExceptionScope scope(&callFrame->vm());
@@ -456,13 +454,4 @@
         RELEASE_ASSERT(!visitor->isInlinedFrame());
 #endif
-        lexicalEnvironment = callFrame->uncheckedActivation();
-        // Protect against the lexical environment not being created, or the variable still being
-        // initialized to Undefined inside op_enter.
-        if (lexicalEnvironment && lexicalEnvironment.isCell()) {
-            JSLexicalEnvironment* activationObject = jsCast<JSLexicalEnvironment*>(lexicalEnvironment);
-            // Protect against throwing exceptions after tear-off.
-            if (!activationObject->isTornOff())
-                activationObject->tearOff(*scope->vm());
-        }
     }

[Filip Pizlo]

(In reply to comment #7)
> Comment on attachment 241252 [details]
> Proposed patch.
> 
> It feels a bit awkward to program defensively like this just to make the
> fuzzer happy. Programming like this means that we can't tell the difference
> between "Something is seriously wrong because the activation object is
> missing" and "I'm just fuzzing".
> 
> Ideally, we would teach the fuzzer not to throw in cases that otherwise
> couldn't -- for example, in the LLInt, by passing an argument to END() that
> said "ASSERT there is no exception, and do not fuzz for exceptions".
> 
> I guess this patch is an improvement for now, so it's worth doing. Note,
> though that you missed a spot: Oliver removed the original work-around for
> fuzzing, probably because he wasn't aware of this special fuzzing behavior.
> 
> You should update your comments to specify that we do this only for fuzzing,
> and also add back the code that Oliver removed in
> <http://trac.webkit.org/changeset/174226>:
> 
> Index: /trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp
> ===================================================================
> --- /trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp	(revision
> 174225)
> +++ /trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp	(revision
> 174226)
> @@ -440,6 +440,4 @@
>      CallFrame* callFrame = visitor->callFrame();
>      CodeBlock* codeBlock = visitor->codeBlock();
> -    JSScope* scope = callFrame->scope();
> -
>      if (Debugger* debugger = callFrame->vmEntryGlobalObject()->debugger()) {
>          ClearExceptionScope scope(&callFrame->vm());
> @@ -456,13 +454,4 @@
>          RELEASE_ASSERT(!visitor->isInlinedFrame());
>  #endif
> -        lexicalEnvironment = callFrame->uncheckedActivation();
> -        // Protect against the lexical environment not being created, or
> the variable still being
> -        // initialized to Undefined inside op_enter.
> -        if (lexicalEnvironment && lexicalEnvironment.isCell()) {
> -            JSLexicalEnvironment* activationObject =
> jsCast<JSLexicalEnvironment*>(lexicalEnvironment);
> -            // Protect against throwing exceptions after tear-off.
> -            if (!activationObject->isTornOff())
> -                activationObject->tearOff(*scope->vm());
> -        }
>      }

For what it's worth, when we added the fuzzer we added a handful of such checks that are only to defend against the fuzzer. This involved far less infrastructure than making the fuzzer more complicated. 

Also, if we ever were wrong about our assumptions about when activations are allocated or who could throw exceptions, this defensiveness would turn a crash into something much less severe.

[Akos Kiss]

Created attachment 241354 [details]
Updated patch

Updated/added comments as requested.

However, I don't think that the change to Interpreter.cpp could/should be reverted. The code that was removed there was callinf isTornOff and tearOff on LexicalEnvironment. However, that API/functionality does not exists anymore.

[Geoffrey Garen]

> However, I don't think that the change to Interpreter.cpp could/should be
> reverted. The code that was removed there was callinf isTornOff and tearOff
> on LexicalEnvironment. However, that API/functionality does not exists
> anymore.

Aha! You are correct.

[Geoffrey Garen]

Comment on attachment 241354 [details]
Updated patch

r=me

[WebKit Commit Bot]

Comment on attachment 241354 [details]
Updated patch

Clearing flags on attachment: 241354

Committed r175967: <http://trac.webkit.org/changeset/175967>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.