494084 - NJ: skip limit is wrong, again.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Graydon Hoare :graydon]

Attachment: Lower the skip limit, factor the constant out, consolidate ensureRoom calls

Error discovered while diagnosing bug 493991.

I should have caught this during review, but the skip limit set on the widen-LIR patch was wrong. One too high, definitely; two too high if we want to be conservative and co-locate skip instructions with skip-blobs on the same page (njn favours this, as do I). When this is wrong we're open to arbitrary writes.

Attached patch also forces this keeping-together of skip+blob by consolidating the calls to ensureRoom inside skip().

This does not block 1.9.1, as it's only an issue on TM/trunk.

Comment 1

[Andreas Gal :gal]

Comment on attachment 378731 [details] [diff] [review]
Lower the skip limit, factor the constant out, consolidate ensureRoom calls

If this is wrong again, I want a refund. How did you find it? Fuzzing using your assembler?

Comment 2

[Graydon Hoare :graydon]

(In reply to comment #1)
> (From update of attachment 378731 [details] [diff] [review])
> If this is wrong again, I want a refund. How did you find it? Fuzzing using
> your assembler?

Yes. For some definition of "fuzzing". As before, it's triggered by any skip of the offending size.

You'll laugh, or cry, but njn just found *another*. Perhaps it's our Last Culprit? We're not incorporating the page header size into the limit either. And *that* affects 1.9.1 as well. Refreshing here and there.

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #2)
> You'll laugh, or cry, but njn just found *another*. Perhaps it's our Last
> Culprit? We're not incorporating the page header size into the limit either.
> And *that* affects 1.9.1 as well. Refreshing here and there.

I laughed *and* cried -- is that bug 493991, or is there another one floating around that I need to worry about for 1.9.1?

Comment 4

[Graydon Hoare :graydon]

(In reply to comment #3)

> I laughed *and* cried -- is that bug 493991, or is there another one floating
> around that I need to worry about for 1.9.1?

Incorporated into the patch for bug 493991; I'll refresh this one to incorporate it here as well, once that lands.

Comment 5

[Graydon Hoare :graydon]

Attachment: Update the patch

Comment 6

[Andreas Gal :gal]

Comment on attachment 379007 [details] [diff] [review]
Update the patch

We should probably consider increasing the page size if we keep chipping away at the skip block size.

Comment 7

[Graydon Hoare :graydon]

http://hg.mozilla.org/tracemonkey/rev/ea609649d536

(Does not need to be landed on 1.9.1, bug 493991 handled it there)

Comment 8

[Nicholas Nethercote [inactive]]

Graydon, you won't like this, but... if we pass insSkip() MAX_SIZE_BYTES, then ensureRoom() is called with an argument that is equivalent to MAX_SIZE_BYTES+sizeof(LIns).  (The units are different, but that's the effect.)

The assertion in ensureRoom() can then be triggered.  To see it in action, just change one of the insSkip() calls in jstracer.cpp to pass MAX_SKIP_BYTES.

Instead of this:

		NanoAssert(count * sizeof(LIns) <= MAX_SKIP_BYTES);

I think it should be this:

		NanoAssert(count * sizeof(LIns) <= MAX_SKIP_BYTES + sizeof(LIns));

In other words, insSkip() is asserting about the skip payload size, but ensureRoom() is asserting about the skip payload + instruction size.

The same thing may apply to your recent change on the 1.9.1 branch.

The number of mistakes made here indicates that the way this is handled needs improvement.  I'll file a couple of follow-up bugs about that.

Comment 9

[Graydon Hoare :graydon]

Curses! Incidentally, this is because I took your advice and merged the two ensureRoom calls. But of course, it seems I had already qpop'ed the fuzzer *just* after I made that change and forgot to re-check after that 1 line change. It asserts immediately :(

(I *do* have the fuzzer set to regularly hit the skip limit..)

This doesn't apply to 1.9.1, for anyone watching. And doesn't actually represent a "serious" bug that will hurt anyone on a nightly, just trip an assert that's too tight in debug builds. Will correct shortly.

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ea609649d536