Bug 154548 – InternalFunction::createSubclassStructure doesn't take into account that get() might throw

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

This leads to bad things.
It does:
```
JSObject* prototype  = jsDynamicCast<JSObject*>(get(blah));
if (prototype) {
   ...
}
```
but, the return result from get() is always invalid when we throw an exception.
We often have code that does this:
```
return throwVMTypeError(blah);
```

So, the above code snippet will successfully cast the result as an object, which leads to total failure because an exception was thrown.

[Saam Barati]

Created attachment 271956 [details]
WIP

[Keith Miller]

Comment on attachment 271956 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=271956&action=review

Looks good overall, one minor nit though.

> Source/JavaScriptCore/runtime/StringConstructor.cpp:137
> +    return JSValue::encode(StringObject::create(vm, structure, exec->uncheckedArgument(0).toString(exec)));

I think this would be cleaner as:

Structure* structure = InternalFunction::createSubclassStructure(exec, exec->newTarget(), globalObject->stringObjectStructure());
if (exec->hadException())
    return JSValue::encode(JSValue());

if (!exec->argumentCount())
    return JSValue::encode(StringObject::create(vm, InternalFunction::createSubclassStructure(exec, exec->newTarget(), globalObject->stringObjectStructure())));

return JSValue::encode(StringObject::create(vm, InternalFunction::createSubclassStructure(exec, exec->newTarget(), globalObject->stringObjectStructure()), exec->uncheckedArgument(0).toString(exec)));

[Keith Miller]

(In reply to comment #2)
> Comment on attachment 271956 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271956&action=review
> 
> Looks good overall, one minor nit though.
> 
> > Source/JavaScriptCore/runtime/StringConstructor.cpp:137
> > +    return JSValue::encode(StringObject::create(vm, structure, exec->uncheckedArgument(0).toString(exec)));
> 
> I think this would be cleaner as:
> 
> Structure* structure = InternalFunction::createSubclassStructure(exec,
> exec->newTarget(), globalObject->stringObjectStructure());
> if (exec->hadException())
>     return JSValue::encode(JSValue());
> 
> if (!exec->argumentCount())
>     return JSValue::encode(StringObject::create(vm,
> InternalFunction::createSubclassStructure(exec, exec->newTarget(),
> globalObject->stringObjectStructure())));
> 
> return JSValue::encode(StringObject::create(vm,
> InternalFunction::createSubclassStructure(exec, exec->newTarget(),
> globalObject->stringObjectStructure()),
> exec->uncheckedArgument(0).toString(exec)));

Whoops, this should be: 

Structure* structure = InternalFunction::createSubclassStructure(exec, exec->newTarget(), globalObject->stringObjectStructure());
if (exec->hadException())
    return JSValue::encode(JSValue());

if (!exec->argumentCount())
    return JSValue::encode(StringObject::create(vm, structure));

return JSValue::encode(StringObject::create(vm, structure, exec->uncheckedArgument(0).toString(exec)));

[Saam Barati]

Created attachment 271959 [details]
patch

[WebKit Commit Bot]

Attachment 271959 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/RegExpConstructor.cpp:270:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/RegExpConstructor.cpp:272:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 271959 [details]
patch

r=me

[Mark Lam]

Comment on attachment 271959 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271959&action=review

> Source/JavaScriptCore/tests/stress/create-subclass-structure-might-throw.js:6
> +let targets = [Function, String, Array];

Why not also test WeakSet, WeakMap, and the other types that you fixed in this patch?

[Mark Lam]

Comment on attachment 271959 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271959&action=review

> Source/JavaScriptCore/runtime/JSGlobalObject.h:735
> +        if (exec->hadException())
> +            return nullptr;

This exception check should be for both the if case as well because arrayStructureForProfileDuringAllocation() calls arrayStructureForIndexingTypeDuringAllocation().

[Mark Lam]

Comment on attachment 271959 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271959&action=review

> Source/JavaScriptCore/runtime/InternalFunction.cpp:105
>              JSObject* prototype = jsDynamicCast<JSObject*>(newTarget.get(exec, exec->propertyNames().prototype));
> +            if (exec->hadException())
> +                return nullptr;

In the if case above, we're also casting newTarget.get() to get a prototype.  You can at least assert that it won't have an exception if it should not throw.

[Saam Barati]

Created attachment 271960 [details]
patch

fixed suggestions

[Saam Barati]

Created attachment 271962 [details]
patch

[Geoffrey Garen]

Comment on attachment 271962 [details]
patch

r=me

[Mark Lam]

Comment on attachment 271962 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271962&action=review

r=me too

> Source/JavaScriptCore/tests/stress/create-subclass-structure-might-throw.js:6
> +let targets = [Function, String, Array, Set, Map, WeakSet, WeakMap, RegExp, Number, Promise, Date, Boolean, Error, TypeError, SyntaxError, ArrayBuffer, Int32Array, Int8Array, Uint8Array, Uint8ClampedArray, Int16Array, Uint16Array, Uint32Array, Float32Array, Float64Array];

I think DataView is made the same way as the TypeArrays.  Maybe you should add that.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/196966