508617 - MethodEnv::initproperty can inappropriately reject const initialization

Status: VERIFIED FIXED

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

Description

[Steven Johnson]

given:

	class A
	{
		private const foo:Object = new Object();
		
		public function A()
		{
		}
	}

	class B extends A
	{
		public function B()
		{
			super();
		}
	}

	var b:B = new B();

this runs correctly with JIT enabled, but in interp-only mode, fails (incorrectly) with

ReferenceError: Error #1074: Illegal write to read-only property foo.as$1:A::foo on foo.as$1.B

Comment 1

[Edwin Smith]

something like

class A
{
    private const foo:Object
    
    public function A() {
        this["foo"] = new Object()
    }
}

could trigger the same failure with JIT enabled, by circumventing early binding in the JIT.

Comment 2

[Steven Johnson]

(In reply to comment #1)
> something like
> 
> class A
> {
>     private const foo:Object
> 
>     public function A() {
>         this["foo"] = new Object()
>     }
> }
> 
> could trigger the same failure with JIT enabled, by circumventing early binding
> in the JIT.

maybe, but it doesn't seem to, regardless of jit mode

Comment 3

[Steven Johnson]

Attachment: Patch

This patch fixes the misbehavior. (It also deletes some unrelated stale code from MNHT).

Note that this might cause previously-function programs to fail.

Comment 4

[Brent Baker]

Attachment: Testcase

This patch should be pushed in with the fix.

Testcase currently fails with "ReferenceError: Error #1074" when running with -Dinterp

Comment 5

[Steven Johnson]

Comment on attachment 393185 [details] [diff] [review]
Testcase

an equivalent testcase is actually already be included in the first patch above. (isn't it?)

Comment 6

[Brent Baker]

Comment on attachment 393185 [details] [diff] [review]
Testcase

Sorry, didn't even look at the first patch.

Comment 7

[Dan Smith]

(In reply to comment #3)
> 
> Note that this might cause previously-function programs to fail.

Steven, how could this patch cause a failure on previously functioning code?   The interpreter code would return an (incorrect) error and the JIT would allow the access.  What's the failure situation?

Comment 8

[Steven Johnson]

(In reply to comment #7)
> (In reply to comment #3)
> > 
> > Note that this might cause previously-function programs to fail.
> 
> Steven, how could this patch cause a failure on previously functioning code?  
> The interpreter code would return an (incorrect) error and the JIT would allow
> the access.  What's the failure situation?

If the code *expected* an exception to be thrown, and it no longer was, it might fail. (This is a pretty unlikely scenario since it's interp-only, but still...)

Comment 9

[Edwin Smith]

Comment on attachment 393059 [details] [diff] [review]
Patch

looks good and also removes MH_CACHE1 which we discussed recently.

Comment 10

[Steven Johnson]

pushed as changeset:   2516:d18863e7f76c

Comment 12

[Chris Peyer]

Verified fixed w/ avm 2780; Initial patch also pushed testcase: test/acceptance/misc/bug_508617.as

Comment 13

[Edwin Smith]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #3)
> > > 
> > > Note that this might cause previously-function programs to fail.
> > 
> > Steven, how could this patch cause a failure on previously functioning code?  
> > The interpreter code would return an (incorrect) error and the JIT would allow
> > the access.  What's the failure situation?
> 
> If the code *expected* an exception to be thrown, and it no longer was, it
> might fail. (This is a pretty unlikely scenario since it's interp-only, but
> still...)

It is unlikely for plain functions to be interpreted in practice, but it is very likely for init code (global code and class static init code) to be interpreted.  so the most likely place to consider for breaking existing code (by suppressing the incorrect exception) would be code that initializes static const class members or global consts.