Bug 174411 – GenericArguments consults the wrong state when tracking modified argument descriptors and mapped arguments

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

[Saam Barati]

This JS program crashes in a debug build b/c we read arbitrary attributes from uninitialized memory:

```
function foo(x) {
    function bar() {
        return x;
    }
    Object.defineProperty(arguments, "0", {enumerable: false, value:45});
    print("Delete result: ", delete arguments[0]);
    return arguments[0];
}
print(foo(20));
```

[Saam Barati]

<rdar://problem/31696186>

[Saam Barati]

Created attachment 315208 [details]
patch

[Yusuke Suzuki]

Comment on attachment 315208 [details]
patch

r=me, nice catch!

[Mark Lam]

Comment on attachment 315208 [details]
patch

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

r=me too

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:195
> +        result = Base::deletePropertyByIndex(cell, exec, index);

Why not just return Base::deletePropertyByIndex(cell, exec, index) directly here instead of setting result.  With that, you can get rid of result, get rid of the else case, and always return true at the bottom of the function.

If you check for !thisObject->isModifiedArgumentDescriptor(index) instead in the if condition above, then you can also minimize the code change here (restore lines below).

[Build Bot]

Comment on attachment 315208 [details]
patch

Attachment 315208 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4106659

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-value-interactions.html

[Build Bot]

Created attachment 315236 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Caio Lima]

Comment on attachment 315208 [details]
patch

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

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:91
> +        ASSERT(result);

Good. I think you need to do the same in "getOwnPropertySlot".

[Saam Barati]

Comment on attachment 315208 [details]
patch

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

>> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:91
>> +        ASSERT(result);
> 
> Good. I think you need to do the same in "getOwnPropertySlot".

Yeah. I’ll make it call this function for consistency

[Saam Barati]

Created attachment 315265 [details]
patch for landing

[Saam Barati]

Looks like this is breaking some test262 tests.

[Saam Barati]

Ok, this patch is more subtle than I thought. I'm wrong in a few ways. For example, we can't unconditionally unmap the argument, because the delete might fail. Also, unconditionally calling unmap is wrong because it doesn't do a bounds check. So I need to do a bounds check before calling it.

[Saam Barati]

Created attachment 315278 [details]
patch

Back up for review since I changed some stuff.

[Mark Lam]

Comment on attachment 315278 [details]
patch

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

r=me

> JSTests/stress/generic-arguments-correct-delete-behavior.js:37
> +functions.push(makeTest(true, false, true));
> +functions.push(makeTest(false, false, true));
> +functions.push(makeTest(true, false, false));
> +functions.push(makeTest(false, false, false));

nit: please swap lines 34-35 with lines 36-37.  That makes it a bit easier to follow the permutation of values.

[Saam Barati]

Created attachment 315287 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 315287 [details]
patch for landing

Rejecting attachment 315287 [details] from commit-queue.

New failing tests:
storage/indexeddb/modern/new-database-after-user-delete.html
Full output: http://webkit-queues.webkit.org/results/4109692

[WebKit Commit Bot]

Created attachment 315293 [details]
Archive of layout-test-results from webkit-cq-01 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[WebKit Commit Bot]

Comment on attachment 315287 [details]
patch for landing

Clearing flags on attachment: 315287

Committed r219433: <http://trac.webkit.org/changeset/219433>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.