[Gnash-dev] stack underruns handling

[strk]

This is a long mail, if you want to jump to the point scroll
down to the *final question* line :)

While fixing handling of malformed SWF produced by Ming (delete/delete2).
I found out that our current handling of "Stack underrun" seems to differ
from the one in the MM player. I'm saying this because it is weird that
nobody noticed the Ming bug in the past, so I'm sure the MM player
was handling that specific malformation in a different way.

In particular, when Gnash is in a stack underrun condition, that
means it expects to find N elements on the stack but only finds
N-M items there. In this case Gnash pushes M undefined values there.

Since TAG arguments are put on the stack in reverse order (1 argument
last), this results in pushing undefined values as first argument
rather then as last.

With ActionDelete (0x3A), this results in a different handling of the bogus SWF.
ActionDelete is supposed to do:

        // Stack: OBJECT, FIELD
        pop FIELD
        pop OBJECT
        delete OBJECT.FIELD

With the bogus Ming version, the stack lacks the OBJECT part, as in:

        // Stack: FIELD
        pop FIELD -- ok!
        pop OBJECT -- not there!

What happens with the MM player (most likely) is that the second
pop returns an UNDEFINED value, and this might also be what Gnash
was doing some time ago (in 0.7.1 - before stack underrun handling
was introduced). So for MM player this becomes:

        // Stack: FIELD
        pop FIELD -- ok !
        pop OBJECT -- get UNDEFINED
        delete FIELD (undefined object == _root)

With the new stack underrun handlign code in Gnash, this becomes:

        // Stack: FIELD
        <stack underrun detected, fixing>
        // Stack: FIELD, UNDEFINED
        pop FIELD -- get UNDEFINED: wrong !
        pop OBJECT -- get FIELD: wrong !
        delete FIELD.UNDEFINED : wrong !


The whole problem with Gnash on this aspect is use of
as_environment::top(N) to get elements of the stack rather
then use of ::pop() and ::push().

top(N) returns an as_value by reference. Previous versions
of Gnash were returning a reference to a static as_value, initially
undefined (!!). That was dangerous and confusing for many reasons, 
including the fact that some tags handler might have changed that
static variable value from undefined to something else when
trying to set the return value (remember, tag handlers often
use top(x).set_y(y) to return!). Also, that never fixed the stack.

I guess top(N) is used trying to be more performant and avoid
continuos memory allocation/deallocation due to pop/push on
the stack, but to handle malformed SWF this seems to get
more complex.

In particular, if we change the fix_stack_underrun() function
to add undefined values *before* the exiting ones, as in:

        // Stack: FIELD
        <stack underrun detected, fixing>
        // Stack: UNDEFINED, FIELD

Any preexisting reference to top(0) would silently now point
to memory address that might as well been deallocated
(think about reallocating an array). The result might be a *disaster*.
Actually, now that I think about this, if the array is really deallocated
the same problem applies now, when pushing elements *after* it.

*final question*

So, the final question at this point is: do you think it would be a big
problem to remove the top(x) method from as_environment and force use
of pop() instead ?

pop() would return by value, while top(x) returns by reference opening
a can of worms, but debatebly being faster.

Comments ?