Re: [Gnash-dev] stack underruns handling

[strk]

It seems nobody read this mail, anyway here's a quick update.
I've committed the "push_before" approach for stack underrun fixes.
top() and pop() methods are still there, so the possible corruptions
are all still pending.

--strk;

On Thu, Dec 14, 2006 at 03:45:02PM +0100, strk wrote:
> 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 ?
> 
> 
> _______________________________________________
> Gnash-dev mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/gnash-dev

-- 

 /"\    ASCII Ribbon Campaign
 \ /    Respect for low technology.
  X     Keep e-mail messages readable by any computer system.
 / \    Keep it ASCII.