OS#17614914 - SCCLiveness::ProcessStackSymUse lifetime nullptr deref #5395

paolosevMSFT · 2018-06-29T15:56:32Z

In bug 17614914 JIT is causing a bad code gen, but the root problem is that with a script like:

Object.prototype.length = undefined;
var ary = new Array();
var func0 = function() {
    for (var _strvar2 in ary) {
        console.log(_strvar2);
    }
};
func0();

a built-in property like Array.length should not be enumerated even if it is defined in Object.prototype.
This PR fixes this problem by ignoring the builtin properties in ForInObjectEnumerator::MoveAndGetNext().

… when they are defined in Object.prototype

MSLaguana · 2018-06-29T16:02:34Z

lib/Runtime/Library/ForInObjectEnumerator.cpp

+                    Assert(specialPropertyIds != nullptr);
+                    for (uint i = 0; i < specialPropertyCount; i++)
+                    {
+                        TestAndSetEnumerated(specialPropertyIds[i]);


Are all special properties non-enumerable?

v8:

Object.getOwnPropertyDescriptors(new String())
{length: {…}}
length: {value: 0, writable: false, enumerable: false, configurable: false}
proto: Object

Object.getOwnPropertyDescriptors(new Function())
{length: {…}, name: {…}, arguments: {…}, caller: {…}, prototype: {…}}
arguments: {value: null, writable: false, enumerable: false, configurable: false}
caller: {value: null, writable: false, enumerable: false, configurable: false}
length: {value: 0, writable: false, enumerable: false, configurable: true}
name: {value: "anonymous", writable: false, enumerable: false, configurable: true}
prototype: {value: {…}, writable: true, enumerable: false, configurable: false}
proto: Object

Object.getOwnPropertyDescriptors(new Array())
{length: {…}}
length: {value: 0, writable: true, enumerable: false, configurable: false}
proto: Object

Object.getOwnPropertyDescriptors(new RegExp())
{lastIndex: {…}}
lastIndex: {value: 0, writable: true, enumerable: false, configurable: false}
proto: Object

Looks good then :)

Chakra:

Object.getOwnPropertyDescriptors(new String())

length: Object

configurable: false

enumerable: false

value: 0

writable: false

Object.getOwnPropertyDescriptors(new Function())

arguments: Object

configurable: false

enumerable: false

value: null

writable: false

caller: Object

configurable: false

enumerable: false

value: null

writable: false

length: Object

configurable: true

enumerable: false

value: 0

writable: false

name: Object

configurable: true

enumerable: false

value: "anonymous"

writable: false

prototype: Object

configurable: false

enumerable: false

value: Object

writable: true

Object.getOwnPropertyDescriptors(new Array())

length: Object

configurable: false

enumerable: false

value: 0

writable: true

Object.getOwnPropertyDescriptors(new RegExp())
global: Object

configurable: false

enumerable: false

value: false

writable: false

ignoreCase: Object

configurable: false

enumerable: false

value: false

writable: false

lastIndex: Object

configurable: false

enumerable: false

value: 0

writable: true

multiline: Object

configurable: false

enumerable: false

value: false

writable: false

options: Object

configurable: false

enumerable: false

value: ""

writable: false

source: Object

configurable: false

enumerable: false

value: "(?:)"

writable: false

sticky: Object

configurable: false

enumerable: false

value: false

writable: false

unicode: Object

configurable: false

enumerable: false

value: false

writable: false

Note that Chakra lists several RegExp properties not enumerated by V8.

All special properties are non-enumerable, as far as I can tell.

pleath · 2018-06-29T17:02:37Z

Is this only an issue for special properties? What about non-enumerable user properties? Should they also shadow enumerable properties in the prototype chain, and are we doing the right thing?

paolosevMSFT · 2018-06-29T17:27:49Z

The spec https://tc39.github.io/ecma262/#sec-enumerate-object-properties states that non-enumerable user properties should shadow enumerable properties in the prototype chain. Chakra already behaves correctly:

var a = {foo: 42};
 
var b = {bar: 11};
Object.defineProperty(b, "foo", { value: 1, enumerable: false });
b.__proto__ = a;
 
for (var p in b) { console.log(p) }

only lists 'bar'.

The logic is handled correctly in ForInObjectEnumerator::MoveAndGetNext(), non-enumerable user properties are correctly enumerated, their enumerability attribute is checked (line 200) and non-enumerable properties are not yielded but still are inserted into a Set (TestAndSetEnumerated, line 199) to shadow the same property in prototypes.

Apparently the only thing not working was that special properties were not enumerated, and therefore TestAndSetEnumerated() was not called for them and there was no shadowing.

Cellule · 2018-06-29T17:58:32Z

lib/Runtime/Library/ForInObjectEnumerator.cpp

                    return nullptr;
                }

+                // Ignore special properties (ex: Array.length)


Could you move this down after the last loop?
We only need to mark the special properties if there is something else to enumerate (ie non-null prototype).

Basically, if all the following checks haven't returned null, then we will proceed to enumerate the prototype.

pleath · 2018-06-29T17:59:01Z

I see. Then if the calls to enumerator.MoveAndGetNext had yielded the special properties to ForInObjectEnumerator::MoveAndGetNext, would the right thing have fallen out from the existing logic? Is it clear why we're not doing that?

Cellule · 2018-06-29T18:09:03Z

I see. Then if the calls to enumerator.MoveAndGetNext had yielded the special properties to ForInObjectEnumerator::MoveAndGetNext, would the right thing have fallen out from the existing logic? Is it clear why we're not doing that?

Yes, if MoveAndGetNext had yield special properties as non-enumerable everything would be fine.
I'm not 100% sure why we don't do that, I wasn't convinced that digging into this was the right way to fix this problem. I think it just has to do with how we attach special properties and that in general we want them to be hidden. Except in this case where they must hide something else.

pleath · 2018-06-29T18:16:01Z

For most purposes, built-in properties have nothing special about them. I'd rather see us make our enumeration logic handle them in a non-special way than invent something new for them, or if there's a reason why that's bad or impossible, at least find out what the reason is.

paolosevMSFT · 2018-06-29T18:57:47Z

How should the special properties be added to be enumerated like the other properties? They should not go in the prototype, right? (I see that JavascriptLibrary::InitializeTypedArrayPrototype() adds a property PropertyIds::length, but JavascriptLibrary::InitializeArrayPrototype() does not).

For example I see that Function objects may be created with builtin properties, like:

    DynamicType * JavascriptLibrary::CreateFunctionWithLengthAndNameType(DynamicObject * prototype, FunctionInfo * functionInfo)
    {
        Assert(!functionInfo->HasBody());
        return DynamicType::New(scriptContext, TypeIds_Function, prototype,
            this->inProfileMode ? ProfileEntryThunk : functionInfo->GetOriginalEntryPoint(),
            &SharedFunctionWithLengthAndNameTypeHandler);
    }

where

    SimpleTypeHandler<2> JavascriptLibrary::SharedFunctionWithLengthAndNameTypeHandler(NO_WRITE_BARRIER_TAG(FunctionWithLengthAndNameTypeDescriptors));


    SimplePropertyDescriptor const JavascriptLibrary::FunctionWithLengthAndNameTypeDescriptors[2] =
    {
        SimplePropertyDescriptor(NO_WRITE_BARRIER_TAG(BuiltInPropertyRecords::length), PropertyConfigurable),
        SimplePropertyDescriptor(NO_WRITE_BARRIER_TAG(BuiltInPropertyRecords::name), PropertyConfigurable)
    };

while for Array object we have just:

INIT_SIMPLE_TYPE(arrayType, TypeIds_Array, arrayPrototype);

and no builtin properties. Should we add builtin properties ('length') here?

Who Is the best person who knows this code well and can point me in the right direction?

pleath · 2018-06-29T19:48:49Z

I see. So the problem with something like Array.length is that it's not represented "normally" in the type system, so the DynamicTypeHandler::FindNextProperty method doesn't see it? In that case, I agree, we need to create something special to deal with it. It would be great to eliminate outliers like this, but perhaps not today (/smile/).

…mUse lifetime nullptr deref Merge pull request #5395 from paolosevMSFT:17614914 In bug 17614914 JIT is causing a bad code gen, but the root problem is that with a script like: ``` Object.prototype.length = undefined; var ary = new Array(); var func0 = function() { for (var _strvar2 in ary) { console.log(_strvar2); } }; func0(); ``` a built-in property like Array.length should not be enumerated even if it is defined in Object.prototype. This PR fixes this problem by ignoring the builtin properties in ForInObjectEnumerator::MoveAndGetNext().

…ProcessStackSymUse lifetime nullptr deref Merge pull request #5395 from paolosevMSFT:17614914 In bug 17614914 JIT is causing a bad code gen, but the root problem is that with a script like: ``` Object.prototype.length = undefined; var ary = new Array(); var func0 = function() { for (var _strvar2 in ary) { console.log(_strvar2); } }; func0(); ``` a built-in property like Array.length should not be enumerated even if it is defined in Object.prototype. This PR fixes this problem by ignoring the builtin properties in ForInObjectEnumerator::MoveAndGetNext().

Built-in properties (like Array.length) should not be enumerated even…

eaa14a7

… when they are defined in Object.prototype

paolosevMSFT changed the title ~~Built-in properties (like Array.length) should not be enumerated even when they are defined in Object.prototype~~ OS#17614914 - SCCLiveness::ProcessStackSymUse lifetime nullptr deref Jun 29, 2018

paolosevMSFT requested review from Cellule and MSLaguana June 29, 2018 16:00

paolosevMSFT mentioned this pull request Jun 29, 2018

OS#17614914 - SCCLiveness::ProcessStackSymUse lifetime nullptr deref #5330

Closed

MSLaguana reviewed Jun 29, 2018

View reviewed changes

MSLaguana approved these changes Jun 29, 2018

View reviewed changes

Cellule reviewed Jun 29, 2018

View reviewed changes

pleath approved these changes Jun 29, 2018

View reviewed changes

Address review comments

89f80cb

Cellule approved these changes Jun 29, 2018

View reviewed changes

chakrabot merged commit 89f80cb into chakra-core:release/1.10 Jun 29, 2018

OS#17614914 - SCCLiveness::ProcessStackSymUse lifetime nullptr deref #5395

OS#17614914 - SCCLiveness::ProcessStackSymUse lifetime nullptr deref #5395

Uh oh!

Conversation

paolosevMSFT commented Jun 29, 2018

Uh oh!

MSLaguana Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

paolosevMSFT Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSLaguana Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

paolosevMSFT Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paolosevMSFT Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

pleath commented Jun 29, 2018

Uh oh!

paolosevMSFT commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cellule Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

pleath commented Jun 29, 2018

Uh oh!

Cellule commented Jun 29, 2018

Uh oh!

pleath commented Jun 29, 2018

Uh oh!

paolosevMSFT commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pleath commented Jun 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

paolosevMSFT Jun 29, 2018 •

edited

Loading

paolosevMSFT Jun 29, 2018 •

edited

Loading

paolosevMSFT commented Jun 29, 2018 •

edited

Loading

paolosevMSFT commented Jun 29, 2018 •

edited

Loading