Closed Bug 539766 Opened 14 years ago Closed 14 years ago

Object.defineProperty can set arguments.length without setting the "tampered with" bit

Categories

(Core :: JavaScript Engine, defect, P2)

Other Branch
defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: jorendorff, Assigned: Waldo)

Details

(Whiteboard: [ccbr] fixed-in-tracemonkey)

Attachments

(1 file)

function f() {
    Object.defineProperty(arguments, "length", {value: 4});
    assertEq(arguments.length, 4);
}
f();

Here the assertion fails because the interpreter (in JSOP_ARGCNT) doesn't bother looking at the actual property. It looks at the "tampered with" bit of argobj->fslots[JSSLOT_ARGS_LENGTH], which is clear, and assumes it can use fp->argc.

This can be used to trigger a crash near NULL:

function f() {
    var q = arguments;
    Object.defineProperty(arguments, "length", {value: {}});
    for (var i = 0; i < 20; i++) {
        print(q.length);
    }
}
f();

Related: bug 539553
When defining a data property over top of a "data, but really JSPropertyOp native accessor" property, should we call clasp->delProperty? args_delProperty would set the magic override bit.

/be
I can believe that.
Do the length guards added in bug 539553 fix this bug too? If so we should still probably add this variant testcase to the regression test suite.

If not, is this one also a regression like bug 539553? If so please set status1.9.1 to "unaffected" so we don't have to worry it.

Where does the near-null value come from? Is it always going to be near-null, or would different values for "value" get us somewhere more interesting?
Whiteboard: [sg:critical?]
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
I would doubt the guards there fix this bug.

Since Object.defineProperty is new in 1.9.3, 1.9.1 is unaffected.  Likewise for 1.9.2.

Having not looked, I can't comment on the near-null value.
No longer crashes tm tip -- suggest unrestricting access and lowering sg: rating.

/be
Group: core-security
Whiteboard: [sg:critical?]
blocking2.0: --- → beta1+
Priority: -- → P2
Keywords: crash
Whiteboard: [ccbr]
blocking2.0: beta1+ → beta2+
blocking2.0: beta2+ → betaN+
Keywords: crash
Still correctness bugs lurking here, although I believe nothing more interesting than that:


[jwalden@find-waldo-now tests]$ ../dbg/js -j
js> function test()
{
  var length = "length";
  Object.defineProperty(arguments, length, { value: 17 });
  print(arguments.length);
  print(arguments[length]);
}
js> test()
0
17

Looking at this now to figure out the best course of action...
Attached patch Patch and testsSplinter Review
There are actually quite a few ways this botch is observable, beyond the one instance I noted above: everything that ever relies on the value gotten by js_GetLengthProperty, which is quite a bit!  There are also a couple one-off locations that call isArgsLengthOverridden() directly, where the arguments object might have a bogus override bit.  (Some uses are inside getters/setters that wouldn't be invoked by a redefined arguments.length, because once that happened you'd have to delete arguments.length to get such a property, but you'd then invoke args_delProperty and end up flipping the bit in the process.)

The tests here should cover all the one-off cases, and they cover the first few js_GetLengthProperty uses (the first few before I realized I'd be writing a dozen-odd more intricate tests to cover them all, for not a ton of likely real-world prevention).  More could be written, but I suspect doing so wouldn't win us enough (if any) bug prevention/detection down the road to make it worthwhile.
Attachment #479982 - Flags: review?(brendan)
Comment on attachment 479982 [details] [diff] [review]
Patch and tests

Good general fix, phrased in terms of old JSClass hooks. We truly need to re-code our internal MOP in ES5 terms.

/be
Attachment #479982 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/8577f29c0579
Whiteboard: [ccbr] → [ccbr] fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/8577f29c0579
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.