432239 - Interpreter.interpretLoop has instructionCount increments without calling Context.observeInstructionCount

Status: RESOLVED WORKSFORME

(Rhino Graveyard :: Core, defect)

Description

[Nam Nguyen]

User-Agent:       Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.8.1.14) Gecko/20080421 Firefox/2.0.0.14
Build Identifier: rhino1_7R1


At Interpreter.java:3340 we have:

    case Token.NEW : {
        if (instructionCounting) {
            cx.instructionCount += INVOCATION_COST;
        }

Here, the instructionCount increment is without giving context a chance to monitor the execution like in other cases where the increment is done through method Interpreter.addInstructionCount(...).

The same irregularity is also found with instructions REF_CALL and CALLSPECIAL.

I am not sure the skipping of Context.observeInstructionCount() is intentional, but IMO the inconsistence would create blind spots for observer code.


Reproducible: Always

Steps to Reproduce:
This test code would cause rhino execution get out of memory error, without any possible intervention from context observer:

function Foo1() {
    this.s1 =  String("very looo....ong string #"); 
    ....
    this.s10 =  String(s1 + 9);
}

function Foo2() {
    this.f1 =  new Foo1();
    ....
    this.f10 =  new Foo1();
}

function Foo3() {
    this.f1 =  new Foo2();
    ....
    this.f10 =  new Foo2();
}
.....

function Foo10() {
    this.f1 =  new Foo9();
    ....
    this.f10 =  new Foo9();
}

var huge = new Foo10();


Actual Results:  
OutOfMemoryError

Expected Results:  
Context.observeInstructionCount could be used to prevent memory abuser.


At Interpreter.java:3340 and similar places for REF_CALL and CALLSPECIAL.

    case Token.NEW : {
        if (instructionCounting) {
            addInstructionCount(cx, INVOCATION_COST);
        }


where:
    private static void addInstructionCount(Context cx, int count) {
        cx.instructionCount += count;
        if (cx.instructionCount > cx.instructionThreshold) {
            cx.observeInstructionCount(cx.instructionCount);
            cx.instructionCount = 0;
        }
    }

Comment 1

[Norris Boyd]

I think the current code is correct. The idea is that we only check against thresholds on jumps within frames, and that we then add in all the instructions traversed in that block of code. This way we avoid the performance penalty of checking against the threshold on every instruction. 

Since the various forms of call don't jump *within the frame*, I think all the instructions are properly accounted for. 

The instruction threshold code isn't intended to protect against OutOfMemoryErrors like you show, only against infinite or long-running loops. Your example doesn't actually execute a lot of instructions.