Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#4759 closed defect (fixed)

[PATCH] JS simulation test components should not be able to modify their template and entity data

Reported by: elexis Owned by: elexis
Priority: Should Have Milestone: Alpha 23
Component: Core engine Keywords:
Cc: Patch: Phab:D871

Description (last modified by elexis)

Problem: Template data and the entity ID of the JS simulation components (EDIT: in tests!) should not be modifyable.

In most cases, such modifications come from assuming that a copy of the template data is modified, while in fact the underlying object is modified, implying hidden bugs.

The affected simulation components then often have to add a clone call to not modify the original object.

C++ simulation components are not affected, because they read template data from the ParamNode directly.

Reproduce: Open Attack.js and set this.entity = -1 or this.template.foo = "bar" for example.

Examples: Two examples are r19623 and r20015. These bugs would have been cought with a red error message instead of a hidden gameplay bug if the properties were frozen.

The same phenomenon of unintentionally modifying GUI and simulation data in JS had been observed and fixed in #3647 and #4257.

Responsible code: JS components are created by ScriptComponent.cpp. The template and entity properties are set by CComponentTypeScript::Init.

r8865 actually attempted to freeze those two properties there, but actually didn't or doesn't anymore. (I've checked out that revision and r9000 but cannot compile it due to FCollada errors).

The template is converted from a ParamNode to a JS::Value in EngineScriptConversions.cpp using ScriptInterface::ToJSVal<CParamNode>. This function also calls deepfreezes.

After the template and entity properties are set, the Init function of the JS components is called by CComponentTypeScript::Init using m_ScriptInterface.CallFunctionVoid, which passes through the adventurous NativeWrapperDecls.h and ends up in ScriptInterface::CallFunction_, which just calls JS_CallFunctionName.

Explored solution: By setting the JSPROP_ENUMERATE flag with the patch below, these two properties will become both visible and frozen in the simulation component (so the aim of the ticket would be achieved).

Index: source/simulation2/scripting/ScriptComponent.cpp
===================================================================
--- source/simulation2/scripting/ScriptComponent.cpp	(revision 20098)
+++ source/simulation2/scripting/ScriptComponent.cpp	(working copy)
@@ -43,8 +43,8 @@
 
 void CComponentTypeScript::Init(const CParamNode& paramNode, entity_id_t ent)
 {
-	m_ScriptInterface.SetProperty(m_Instance, "entity", (int)ent, true, false);
-	m_ScriptInterface.SetProperty(m_Instance, "template", paramNode, true, false);
+	m_ScriptInterface.SetProperty(m_Instance, "entity", (int)ent, true, true);
+	m_ScriptInterface.SetProperty(m_Instance, "template", paramNode, true, true);
 	m_ScriptInterface.CallFunctionVoid(m_Instance, "Init");
 }
 

However for...in loop and Object.properties don't list these properties currently, but after setting the flag, they would. So every JS simulation component would have to take care to explicitly ignore these two. The C++ serialization code (including the BinarySerializer.cpp) will have to account for the two new properties encountered.

IF setting the JSPROP_ENUMERATE is not the right way to go, then the frozen template property must be unintentionally copied somewhere by the callstack, which is either a bug on our side or in SpiderMonkey.

Change History (4)

comment:1 by leper, 7 years ago

Reproduce: Open Attack.js and set this.entity = -1 or this.template.foo = "bar" for example.

With

Attack.prototype.Init = function()
{
warn(uneval(this.template));
this.template.bar = 1;
warn(uneval(this.template));
};

I get

WARNING: ({Melee:{Crush:"0.0", Hack:"12.0", MaxRange:"4.0", Pierce:"0.0", PrepareTime:"500", RepeatTime:"1000", RestrictedClasses:{'@datatype':"tokens", _string:"Structure Ship Siege"}}})
ERROR: JavaScript error: simulation/components/Attack.js line 187
TypeError: ({Melee:{Crush:"0.0", Hack:"12.0", MaxRange:"4.0", Pierce:"0.0", PrepareTime:"500", RepeatTime:"1000", RestrictedClasses:{'@datatype':"tokens", _string:"Structure Ship Siege"}}}) is not extensible
  Attack.prototype.Init@simulation/components/Attack.js:187:1

With warning this.entity and setting just that I get

WARNING: 1535
ERROR: JavaScript error: simulation/components/Attack.js line 187
TypeError: "entity" is read-only
  Attack.prototype.Init@simulation/components/Attack.js:187:1

So this seems to be working as expected, or am I missing something?

Last edited 7 years ago by leper (previous) (diff)

comment:2 by elexis, 7 years ago

Description: modified (diff)
Milestone: BacklogAlpha 23
Patch: Phab:D871
Summary: JS simulation components should not be able to modify their template and entity data[PATCH] JS simulation test components should not be able to modify their template and entity data

You are right, I'm glad I can reproduce that ingame! The original error that led to the fix in r19623 was a "read-only" error as well.

The issue exists only within the JS simulation components tests (I thought they were created equally many hours ago).

r20015 made me look at the entire deepfreeze thing (#4257) and it appears like two clones could be unneeded there.

Phab:D871 removes them and freezes the two properties.

Thanks for finding my fallacy, case closed basically!

comment:3 by elexis, 7 years ago

Owner: set to elexis
Resolution: fixed
Status: newclosed

In 20134:

Mark simulation component template and entity properties as read-only in simulation tests (as they are in actual games).

Thus throw errors if a simulation test tries to alternate the template instead of silently passing tests with wrong values (as happened tests fixed by rP20015).
Remove the two clones from simulation components that were only relevant for that test.
Add test to test the test.

Differential Revision: https://code.wildfiregames.com/D871
Fixes #4759
setup.js and setup_test.js Reviewed By: leper

comment:4 by elexis, 6 years ago

In 20989:

Rename expectException from rP20134 to TS_ASSERT_EXCEPTION.
Move it from the test of the testsetup to the setup of the tests, so it can be reused by tests, refs #4759, D871.

Note: See TracTickets for help on using tickets.