[Gnash-commit] gnash ChangeLog server/Property.cpp server/Prop...

[Sandro Santilli]

CVSROOT:        /sources/gnash
Module name:    gnash
Changes by:     Sandro Santilli <strk>  08/02/27 16:22:34

Modified files:
        .              : ChangeLog 
        server         : Property.cpp Property.h 
        testsuite/actionscript.all: Object.as 

Log message:
        don't go into an infinite loop if a getter or setter gets or sets
        the member it targets.  Fixes bug #22420.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/gnash/ChangeLog?cvsroot=gnash&r1=1.5751&r2=1.5752
http://cvs.savannah.gnu.org/viewcvs/gnash/server/Property.cpp?cvsroot=gnash&r1=1.6&r2=1.7
http://cvs.savannah.gnu.org/viewcvs/gnash/server/Property.h?cvsroot=gnash&r1=1.18&r2=1.19
http://cvs.savannah.gnu.org/viewcvs/gnash/testsuite/actionscript.all/Object.as?cvsroot=gnash&r1=1.38&r2=1.39

Patches:
Index: ChangeLog
===================================================================
RCS file: /sources/gnash/gnash/ChangeLog,v
retrieving revision 1.5751
retrieving revision 1.5752
diff -u -b -r1.5751 -r1.5752
--- ChangeLog   27 Feb 2008 12:52:56 -0000      1.5751
+++ ChangeLog   27 Feb 2008 16:22:32 -0000      1.5752
@@ -1,5 +1,15 @@
 2008-02-27 Sandro Santilli <address@hidden>
 
+       * server/Property.{cpp,h}: don't go into an infinite loop
+         if a getter or setter gets or sets the member it targets.
+         Fixes bug #22420.
+       * testsuite/actionscript.all/Object.as: add test for recursive
+         getter/setter. Gnash fails at NOT protecting from infinite
+         recursion non-trival call graphs in SWF7+ (but is compatible
+         with SWF6 in that reguard).
+
+2008-02-27 Sandro Santilli <address@hidden>
+
        * server/as_environment.h: callStackDepth can be static, since
          the call stack is..
        * server/vm/ASHandlers.cpp: add more logging in DefineFunction{2}.

Index: server/Property.cpp
===================================================================
RCS file: /sources/gnash/gnash/server/Property.cpp,v
retrieving revision 1.6
retrieving revision 1.7
diff -u -b -r1.6 -r1.7
--- server/Property.cpp 21 Jan 2008 20:55:47 -0000      1.6
+++ server/Property.cpp 27 Feb 2008 16:22:33 -0000      1.7
@@ -26,6 +26,19 @@
 Property::getDelayedValue(const as_object& this_ptr) const
 {
        const as_accessors* a = boost::get<const as_accessors>(&mBound);
+
+       // Don't recursively invoke a getter
+       //
+       // NOTE: it seems recursion protection is handled differently
+       //       when the SWF is targetted at player 7 or higher.
+       //       See actionscript.all/Object.as
+       //
+       as_accessors::ScopedLock lock(*a);
+       if ( ! lock.obtained() )
+       {
+               return a->underlyingValue;
+       }
+
        as_environment env;
        fn_call fn(const_cast<as_object*>(&this_ptr), &env, 0, 0);
        if (mDestructive)
@@ -56,11 +69,29 @@
 void
 Property::setDelayedValue(as_object& this_ptr, const as_value& value)
 {
-       const as_accessors* a = boost::get<const as_accessors>(&mBound);
+       log_debug("setDelayedValue: %s", value.to_debug_string().c_str());
+       as_accessors* a = boost::get<as_accessors>(&mBound);
+
+       // Don't recursively invoke a setter
+       //
+       // NOTE: it seems recursion protection is handled differently
+       //       when the SWF is targetted at player 7 or higher.
+       //       In particular, it is not avoided unless the setter
+       //       calls self.. See actionscript.all/Object.as
+       //
+       as_accessors::ScopedLock lock(*a);
+       if ( ! lock.obtained() )
+       {
+               a->underlyingValue = value;
+               return;
+       }
+
+
        as_environment env;
        env.push(value);
        fn_call fn(&this_ptr, &env, 1, 0);
        (*a->mSetter)(fn);
+
        return;
 
        // TODO: Push value
@@ -105,8 +136,7 @@
        case 2: // Getter/setter
        {
                const as_accessors& a = boost::get<as_accessors>(mBound);
-               if (a.mGetter) a.mGetter->setReachable();
-               if (a.mSetter) a.mSetter->setReachable();
+               a.markReachableResources();
                break;
        }
        default:
@@ -115,4 +145,12 @@
        }
 }
 
+void
+as_accessors::markReachableResources() const
+{
+       if (mGetter) mGetter->setReachable();
+       if (mSetter) mSetter->setReachable();
+       underlyingValue.setReachable();
+}
+
 } // namespace gnash

Index: server/Property.h
===================================================================
RCS file: /sources/gnash/gnash/server/Property.h,v
retrieving revision 1.18
retrieving revision 1.19
diff -u -b -r1.18 -r1.19
--- server/Property.h   21 Jan 2008 20:55:47 -0000      1.18
+++ server/Property.h   27 Feb 2008 16:22:33 -0000      1.19
@@ -15,7 +15,7 @@
 // along with this program; if not, write to the Free Software
 // Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 
-/* $Id: Property.h,v 1.18 2008/01/21 20:55:47 rsavoye Exp $ */ 
+/* $Id: Property.h,v 1.19 2008/02/27 16:22:33 strk Exp $ */ 
 
 #ifndef GNASH_PROPERTY_H
 #define GNASH_PROPERTY_H
@@ -24,11 +24,13 @@
 #include "gnashconfig.h"
 #endif
 
-#include <boost/variant.hpp>
-
 #include "as_prop_flags.h"
 #include "as_value.h"
 #include "string_table.h"
+#include "log.h"
+
+#include <boost/variant.hpp>
+#include <cassert>
 
 namespace gnash {
 
@@ -39,12 +41,56 @@
 class as_accessors
 {
 public:
+
+       /// For SWF6 (not higher) a setter would not be invoked
+       /// while being set. This ScopedLock helps marking a
+       /// Getter-Setter as being invoked in an exception-safe
+       /// manner.
+       class ScopedLock {
+               const as_accessors& a;
+               bool obtainedLock;
+
+               ScopedLock(ScopedLock&);
+               ScopedLock& operator==(ScopedLock&);
+       public:
+
+               ScopedLock(const as_accessors& na) : a(na)
+               {
+                       if ( a.beingAccessed ) obtainedLock=false;
+                       else {
+                               a.beingAccessed = true;
+                               obtainedLock = true;
+                       }
+               }
+
+               ~ScopedLock() { if ( obtainedLock) a.beingAccessed = false; }
+
+               /// Return true if the lock was obtained
+               //
+               /// If false is returned, we're being called recursively,
+               /// which means we should set the underlyingValue instead
+               /// of calling the setter (for SWF6, again).
+               ///
+               bool obtained() const { return obtainedLock; }
+       };
+
+       friend class ScopedLock;
+
        as_function* mGetter;
        as_function* mSetter;
 
+       as_value underlyingValue;
+
        as_accessors(as_function* getter, as_function* setter) : 
mGetter(getter),
-               mSetter(setter)
+               mSetter(setter), underlyingValue(), beingAccessed(false)
        {/**/}
+
+       void markReachableResources() const;
+
+private:
+
+       mutable bool beingAccessed;
+
 };
 
 /// An abstract property
@@ -186,6 +232,7 @@
                        // Destructive are always overwritten.
                        if (mDestructive)
                        {
+       gnash::log_debug("destructive getter/setter, value %s", 
value.to_debug_string().c_str());
                                mDestructive = false;
                                mBound = value;
                        }

Index: testsuite/actionscript.all/Object.as
===================================================================
RCS file: /sources/gnash/gnash/testsuite/actionscript.all/Object.as,v
retrieving revision 1.38
retrieving revision 1.39
diff -u -b -r1.38 -r1.39
--- testsuite/actionscript.all/Object.as        24 Oct 2007 21:55:33 -0000      
1.38
+++ testsuite/actionscript.all/Object.as        27 Feb 2008 16:22:34 -0000      
1.39
@@ -20,7 +20,7 @@
 // compile this test case with Ming makeswf, and then
 // execute it like this gnash -1 -r 0 -v out.swf
 
-rcsid="$Id: Object.as,v 1.38 2007/10/24 21:55:33 strk Exp $";
+rcsid="$Id: Object.as,v 1.39 2008/02/27 16:22:34 strk Exp $";
 
 #include "check.as"
 
@@ -307,6 +307,79 @@
 check_equals(c, 7);
 check(o.addProperty != Object.prototype.addProperty );
 
+// recursive setter
+mem_setter = function(x)
+{
+       this.mem2 = x;
+       this.mem = x;
+};
+mem_getter = function()
+{
+       return this.mem;
+};
+o = {};
+o.addProperty('mem', mem_getter, mem_setter);
+check_equals(typeof(o.mem), 'undefined');
+o.mem = 3; // watch out for recursion !
+check_equals(o.mem, 3);
+check_equals(o.mem2, 3);
+
+// Test double-recursion of setter:
+//  setter1 triggers setter2
+
+mem1_getter = function() { return this.mem1; };
+mem1_setter = function(x)
+{
+       if ( this.setterCalls > 2 )
+       {
+               return;
+       }
+       ++this.setterCalls;
+       o2.mem2 = x;
+       this.mem1 = x;
+};
+
+mem2_getter = function() { return this.mem2; };
+mem2_setter = function(x)
+{
+       if ( this.setterCalls > 2 )
+       {
+               return;
+       }
+       ++this.setterCalls;
+       o1.mem1 = x;
+       this.mem2 = x;
+};
+
+o1 = {}; o1.addProperty('mem1', mem1_getter, mem1_setter);
+o2 = {}; o2.addProperty('mem2', mem2_getter, mem2_setter);
+
+o1.setterCalls = o2.setterCalls = 0; // reset counters
+o1.mem1 = 3;
+#if OUTPUT_VERSION == 6
+ check_equals(o1.setterCalls, 1);
+ check_equals(o2.setterCalls, 1);
+#else
+ // SWF7+ doesn't protect recursion..
+ xcheck_equals(o1.setterCalls, 3);
+ xcheck_equals(o2.setterCalls, 3);
+#endif
+check_equals(o1.mem1, 3);
+check_equals(o1.mem1, 3);
+
+o1.setterCalls = o2.setterCalls = 0; // reset counters
+o2.mem2 = 6;
+#if OUTPUT_VERSION == 6
+ check_equals(o1.setterCalls, 1);
+ check_equals(o2.setterCalls, 1);
+#else
+ // SWF7+ doesn't protect recursion..
+ xcheck_equals(o1.setterCalls, 3);
+ xcheck_equals(o2.setterCalls, 3);
+#endif
+check_equals(o1.mem1, 6);
+check_equals(o2.mem2, 6);
+
 // Object.addProperty wasn't in SWF5
 #endif // OUTPUT_VERSION > 5
 
@@ -455,8 +528,11 @@
 #endif // OUTPUT_VERSION > 5
 
 
-#if OUTPUT_VERSION > 5
-totals(156);
-#else
+#if OUTPUT_VERSION <= 5
 totals(63);
 #endif
+
+#if OUTPUT_VERSION >= 6
+totals(167);
+#endif
+