544370 - Duplicate-function checking can generate bogus CorruptABC errors

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Steven Johnson]

The fix for jitted invokers (https://bugzilla.mozilla.org/show_bug.cgi?id=529833) caused a subtle difference in the way we check for duplicate functions.

In MethodEnv::startCoerce we check for mismatched ScopeTypeChain, and if they don't agree, we throw a verifyerror. 

The problem is that if a given MethodInfo calls OP_newfunction on itself during verification, it ends up creating a new (but equivalent) ScopeTypeChain. This formerly was not a problem, as the mismatched-Scope-check was done prior to the verify, but now, it's done afterwards.

Comment 1

[Steven Johnson]

Attachment: Patch

Initial patch attempt: this adds some intelligence to makeIntoPrototypeFunction to determine if the new ScopeTypeChain being created is equivalent to the one already in use, and if so, just use the existing one. (Aside from fixing the scope-mismatch check issue, this saves a trivial amount of memory.)

Question: should we simply throw kVerifyError if the scopes aren't mismatched?

Comment 2

[Steven Johnson]

Attachment: Optimization

Here's an optimization (addititive to the previous patch, which was purely a fix) that occurred to me during the fix... this allows us to move the scope-mismatch check out of startCoerce entirely by making a custom invoker function used only for duplicated functions.

Not sure if this optimization is worth the gyrations it involves, so not asking for a review, mostly just capturing the change here to avoid losing it.

Comment 3

[Steven Johnson]

Attachment: Patch #2

Upon reflection and discussion, it appears that this chunk of code is really meant to do two things: (1) reject functions that get called with different scopes, but (2) work around an old ASC bug that would cause some functions to inject an "OP_newfunction" call for themself inside their function body. This patch should allow us the best of both worlds: it accepts the existing SWFs with the redundant OP_newfunction, but rejects fuzzed SWFs that try to use different scopes. And, it remove the call-time check entirely.

Comment 4

[Steven Johnson]

Attachment: Patch #3

Slightly tweaked patch that improves compatibility.

Comment 5

[Edwin Smith]

Comment on attachment 426143 [details] [diff] [review]
Patch #3

Verifier.cpp:757

the expression

  (f->method_id() == int32_t(imm30) && curScope->equals(scope))

is redundant.  when (f->method_id() == imm30), f == info, and therefore curScope == scope.  You could add an assert to that effect and simplify the if.

R+ with that tweak.

Comment 6

[Steven Johnson]

pushed with tweak:

http://hg.mozilla.org/tamarin-redux/rev/d9751032bce9

Comment 7

[Chris Peyer]

verified in argo d9751032bce9 and e498e2cd5da8

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch T: regression test

Regression test derived from reading comment 0; validated by confirming that it fails to verify on rev 3796 and that it successfully verifies on rev 3797.

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #8)
> Created attachment 581929 [details] [diff] [review]
> patch T: regression test
> 
> Regression test derived from reading comment 0; validated by confirming that
> it fails to verify on rev 3796 and that it successfully verifies on rev 3797.

Above described how I validated that this test was "sufficient" for guarding against a regression.  I have now also validated that this test is necessary, by injecting the bug into the current code base and rerunning the acceptance tests; without patch T, all the acceptance tests pass, and thus fail to catch the problem.

Comment 10

[Tamarin Bot]

changeset: 6791:7f9cea0cb693
user:      Felix S Klock II <[e-mail]>
summary:   Bug 544370: regression test (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/7f9cea0cb693