Bug 154548 - InternalFunction::createSubclassStructure doesn't take into account that get() might throw
Summary: InternalFunction::createSubclassStructure doesn't take into account that get(...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-22 12:13 PST by Saam Barati
Modified: 2016-02-22 16:51 PST (History)
11 users (show)

See Also:


Attachments
WIP (17.79 KB, patch)
2016-02-22 14:29 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (21.20 KB, patch)
2016-02-22 15:11 PST, Saam Barati
mark.lam: review-
Details | Formatted Diff | Diff
patch (22.04 KB, patch)
2016-02-22 15:54 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (21.97 KB, patch)
2016-02-22 15:55 PST, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-02-22 12:13:06 PST
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.
Comment 1 Saam Barati 2016-02-22 14:29:06 PST
Created attachment 271956 [details]
WIP
Comment 2 Keith Miller 2016-02-22 14:35:40 PST
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)));
Comment 3 Keith Miller 2016-02-22 14:37:18 PST
(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)));
Comment 4 Saam Barati 2016-02-22 15:11:08 PST
Created attachment 271959 [details]
patch
Comment 5 WebKit Commit Bot 2016-02-22 15:13:33 PST
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.
Comment 6 Andreas Kling 2016-02-22 15:27:12 PST
Comment on attachment 271959 [details]
patch

r=me
Comment 7 Mark Lam 2016-02-22 15:27:49 PST
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?
Comment 8 Mark Lam 2016-02-22 15:33:51 PST
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().
Comment 9 Mark Lam 2016-02-22 15:37:05 PST
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.
Comment 10 Saam Barati 2016-02-22 15:54:26 PST
Created attachment 271960 [details]
patch

fixed suggestions
Comment 11 Saam Barati 2016-02-22 15:55:33 PST
Created attachment 271962 [details]
patch
Comment 12 Geoffrey Garen 2016-02-22 15:59:10 PST
Comment on attachment 271962 [details]
patch

r=me
Comment 13 Mark Lam 2016-02-22 16:00:54 PST
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.
Comment 14 Saam Barati 2016-02-22 16:51:41 PST
landed in:
http://trac.webkit.org/changeset/196966