510905 - Shape should cover JSClass.{add,get,set}Property hooks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

This test case shows how a getter can be cloned to an object of a different JSClass.

function f(){}
f.prototype = tracemonkey.__proto__;
var a = [tracemonkey, new f];              // same shape
for each (obj in a)
    obj.HOTLOOP = 10000;                   // add property with getter to a[1]
assertEq(a[1].HOTLOOP, 10000);             // getter is erroneously called

The first time through the loop, a property cache entry is filled with the sprop of the new property added to a[0]. The second time through, the same sprop is used to add a HOTLOOP property to a[1]. This is wrong. a[1].HOTLOOP should not have a getter; it needs a different sprop.

This isn't possible for all classes, and surprisingly many getters can be cloned without causing any observable bug. The jitstats getter is silly. (This example needs -j because the jitstats object happens to have a getter that illustrates the bug. The JIT itself is not relevant.)

JSClass.setProperty can be cloned in the same way.

Similar tricks can probably cause JSClass.addProperty to be skipped, or called on an object of the wrong type.

Security: Getters already have to be robust against being called with an obj of the wrong class. Non-SHARED setters and addProperty hooks do not, so we might have vulnerabilities there.

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)
> This test case shows how a getter can be cloned to an object of a different
> JSClass.
> 
> function f(){}
> f.prototype = tracemonkey.__proto__;
> var a = [tracemonkey, new f];              // same shape

Different shape once bug 497789 is fixed (this week or bust).

/be

Comment 2

[Jason Orendorff [:jorendorff]]

This is fixed by the patch in bug 505523, I think. At least the test case in comment 0 passes with that patch.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #2)

Since bug 505523 is RESOLVED FIXED, can this bug be closed?

Comment 4

[Jason Orendorff [:jorendorff]]

No, I was wrong in comment 2. Objects of different classes can still have the same shape.

  js> a = []; a.x = 1; shapeOf(a);
  159
  js> b = function(){}; b.x = 1; shapeOf(b);
  159

This presumably means getters can be erroneously cloned as illustrated in the description, but I can't think of a shell example.

Comment 5

[Robert Sayre]

Assigned to Jason for now.

Comment 6

[Brendan Eich [:brendan]]

The patch in bug 497789 makes shape depend on prototype identity via the proto's emptyScope->shape, and empty scope association with proto scope depends on the proto scope's canProvideEmptyScope(ops, clasp) function returning true. That method is mainly:

    return this->ops == ops && (!emptyScope || emptyScope->clasp == clasp);

(ignoring early false return in patch for bug 497789). So the only way the method can return true is if the proto has no empty scope yet, in which case it will have one for clasp created (next line in InitScopeForObject); or the clasp in the empty scope matches the wanted one.

So this bug should be fixed as soon as the patch for bug 497789 lands. Comment 4 test results with shell built with bug 497789's patch applied:

js> a = []; a.x = 1; shapeOf(a);
152
js> b = function(){}; b.x = 1; shapeOf(b);
156

/be

Comment 7

[Brendan Eich [:brendan]]

This is fixed in tm now due to the patch for bug 497789.

/be

Comment 8

[Robert Sayre]

 bug 497789 is fixed.