Re: [Gnash-commit] /srv/bzr/gnash/avm2 r9685: Fix segfault caused by null objects in the scope stack.

[Tom Stellard]

On Sat, Nov 15, 2008 at 3:10 PM, strk <address@hidden> wrote:
> On Sat, Nov 15, 2008 at 11:41:54AM +0800, Tom Stellard wrote:
>> On Sat, Nov 15, 2008 at 4:36 AM, strk <address@hidden> wrote:
>> > On Sat, Nov 15, 2008 at 02:19:01AM +0800, Tom Stellard wrote:
>> >> ------------------------------------------------------------
>> >> revno: 9685
>> >> committer: Tom Stellard <address@hidden>
>> >> branch nick: gnash_dev
>> >> timestamp: Sat 2008-11-15 02:19:01 +0800
>> >> message:
>> >>   Fix segfault caused by null objects in the scope stack.
>> >> modified:
>> >>   libcore/as_environment.cpp
>> >>   libcore/vm/Machine.cpp
>> >
>> >> === modified file 'libcore/as_environment.cpp'
>> >> --- a/libcore/as_environment.cpp      2008-11-06 19:04:35 +0000
>> >> +++ b/libcore/as_environment.cpp      2008-11-14 18:19:01 +0000
>> >> @@ -679,6 +679,7 @@
>> >>                               {
>> >>                                       // const_cast needed due to 
>> >> non-const as_object::get_member
>> >>                                       as_object* obj = 
>> >> const_cast<as_object*>((*scopeStack)[i-1].get());
>> >> +                                     if(!obj) continue;
>> >
>> > Shouldn't we avoid putting NULLs in the scope stack instead ?
>> > How did it get there ?
>> >
>> > --strk;
>> >
>>
>> NULLs are added to the scope stack when AVM2 can't find the object it
>> is looking for.  This usually happens when there is an unimplemented
>> actionscript class, or when AVM2 didn't create an object when it was
>> supposed to.  I think I could probably add code in push_scope_stack()
>> that prevents NULL objects from being added to the scope stack.
>
> The aim here is to find a programmatic error.
> Pushing a NULL in the scope stack would be a programmatic error,
> so silently behaving in a weird way is what you want to avoid.
> The best would be to log an AS error from the caller, whenever
> it's about to push a NULL.
> In this way, you can further inspect the problem producing a
> focused testcase.
>
> Doing so, I found out what the behaviour for AVM1 was in this case.
> AVM1 can push an object to the scope-stack for a block of code,
> using 'with':
>
>        with (myObject)
>        {
>                doThis();
>                doThat();
>        }
>
> By having the scope pusher abort you can easily get a backtrace
> and find the caller. By having the caller log_aserror and avoid
> the push you can easily find movies doing so. Once you find the
> movie you can check the expected behaviour.
> In the 'with (undefined)' case, for example, the expected
> behaviour was to completely skip the inside block!
> I would have had an hard time figuring if I didn't have an AS
> error logged, and an abort to remind me I should have avoided
> the call completely.
>
> --strk;
>

So, what I should be doing is something like:

prop =getProperty();
if(!prop)
    log_aserror();

pushStack(prop);

and if I try to pass a NULL value to pushStack, it should abort.  What
is the preferred way to abort?  Should I throw an exception?

-Tom