Procedures named with Object prototype attr names cause crash upon executing

[apple502j]

Expected Behavior

Scratch should not crash, especially in the thread (because it does not show the blue screen and consume memory by spamming browser console)

Actual Behavior

It crashes

Steps to Reproduce

Make a procedure named "hasOwnProperty" or something else in the object prototype.

Operating System and Browser

any

Fixes

Procedure related cache attributes need to use prototypeless object or Map.

This is different from #2332 because modifying json is not necessary for this to happen.

[aetinx]

When an issue has both "High severity" and "Low impact" tags. LOL.

[benjiwheeler]

When an issue has both "High severity" and "Low impact" tags. LOL.

This combination means "when it happens it's bad, but it seldom happens".

[benjiwheeler]

@apple502j Do you have repro steps? I don't totally understand what the problem is.

[adroitwhiz]

Create a custom block named hasOwnProperty, then run it:

The VM will silently crash and the console will become filled with errors.

[AmazingMech2418]

Can reproduce.

Judging by the errors thrown, it is an internal React error dealing with how this code processes custom blocks. Seems like something might use the name of the custom block with either subscript or dot notation to get a property from an object, and since the property is in the Object prototype, it would already be defined and might throw an error. Clearly not method pollution though given that isPrototypeOf is never used in the code, yet still throws the error.

isPrototypeOf is also never used in the React source code either.

[apple502j]

Wrong analysis; this is probably VM cache misbehaving, e.g. https://github.com/LLK/scratch-vm/blob/develop/src/engine/blocks.js#L228

[AmazingMech2418]

Wrong analysis; this is probably VM cache misbehaving, e.g. https://github.com/LLK/scratch-vm/blob/develop/src/engine/blocks.js#L228

That's the same line I found the error to be in. It uses subscript notation with the key as the name of the custom block, so if it is in the Object prototype, it might throw an error.

[AmazingMech2418]

Actually, sorry, it was https://github.com/LLK/scratch-vm/blob/607cd2d392b0d16eb14e3512de9ac6741f2dfe3d/src/engine/blocks.js#L264 where I found it to be.

[AmazingMech2418]

Wait, the issue is that the procedure is not undefined, but is not defined as it should be.

It is defined in the prototype rather than by scratch-vm, so it treats it as a properly cached name or definition, and when run, it throws an error since it is treating a function as if it was an array.

[AmazingMech2418]

Some ways to fix this include:

• Modifying cache to not use objects with keys, such as maybe a custom dictionary type that stores an array of keys and an array of values, although this would harm performance
• Adding a string to the front and/or end of keys in the cache for custom blocks to prevent accidentally calling a prototype-defined function
• Modifying the cache system to not use the procedure name as the key, but maybe something else, such as an auto-generated opcode for the procedure

There are probably a few other ways, but these are all I can think of off the top of my head.

Edit: Another way is to check if it is not a function too, not just if it is not undefined. This might cause unintended method pollution with future updates though.

[apple502j]

Object.create(null) is enough

[AmazingMech2418]

Object.create(null) is enough

I'm not sure what you mean... The issue is that the names are already defined through the prototype, so it never gets defined as a procedure for the Scratch project.

[travissanderson]

@ericrosenbaum would you be interested in a PR that resolves this issue? I have code that works when I manually test it, as well as unit tests that show the fix works. I was opening a PR and realized the contributing guidelines say to work on "Help Wanted" issues or ask a repo coordinator. Didn't realize this issue was not tagged as Help Wanted yet! Let me know if you'd like me to open a PR for this change https://github.com/LLK/scratch-vm/compare/develop...travissanderson:avoid-collision-with-object-attributes?expand=1

[ericrosenbaum]

@travissanderson I think this is likely an issue that our team would need to work on internally - there are often complex implications of changes like this that can be hard to test. Thanks for your work on it though! If we're able to get to this issue (once we have the capacity) we'll have your work to look at.

[travissanderson]

No problem, totally understand. I’ll make sure to be more diligent about the help wanted label!

[adroitwhiz]

@cwillisf I think the latest set of lint fixes addresses this (might need some testing)