Bug Patch: Callstack Mangling when using Behaviors

That was fun. How about another one?

-----

Code Base: TGB Pro 1.7.5 (should work for 1.7.6 also)

Bug:

Suppose you are calling a script method on an object that has behaviors:

%obj.stop();

And this call leads to the execution of two or more behaviors, we'll call them b1 and b2 for clarity, even though behaviors don't have names.

// the above call will actually execute two behaviors
  %obj.b1.stop();
  %obj.b2.stop();

Or you may have a method than runs on both the behavior and its owner

// the above call actually executes methods on the owner and one behavior
	%obj.b1.stop();
	%obj.stop();

Then whoah is you! At best, only your first method runs correctly, and at worst, you may get a (reproducible) crash. Have you ever re-arranged the order of your statements within a script method and stopped crashing, or, like me, added a spurious "echo" call at the end of a method and suddenly it stopped crashing?

The problem is that the torqueScript interpreter has a "StringStack" to keep info about the strings in your stack of method calls. When this stack needs to be viewed as "argv" data, a shared array of pointers pointing into the StringStack, is used for all methods! When methods call other methods, the data is overwritten. Well, when a program unfurled as a single line of method calls, this probably worked, although it stared over the abyss I'm sure.

But now, with behaviors, you may need to call a method on one or more behaviors and possibly the owner as well. When the script interpreter comes back to run the method in the second behavior, this shared argv has been "shared" and pointed at some other method that was called within the first behavior's execution -- maybe an "echo" or some string params to a "new". Unfortunately, the interpreter does not realize this. In the example above, instead of the shared argv pointing to the args for "stop," it may point to anything. Nastiness ensues. At best, something busted gets silently failed out.

The solution is to NOT use a shared argv anymore.

Patch:

Files Affected:
component/behaviors/behaviorComponent.cpp
console/compiledEval.cc
console/stringStack.cc
console/stringStack.h

Included:
included in-line. The original file name was 'tgb175_callstack_fix_2012-02-08.patch' and it was a "unified diff" meaning it has context in it so that line-numbers aren't as important. It should be patched from your "source/" directory.

Index: component/behaviors/behaviorComponent.cpp
===================================================================
--- component/behaviors/behaviorComponent.cpp	(revision 192)
+++ component/behaviors/behaviorComponent.cpp	(working copy)
@@ -117,14 +117,10 @@
    // about ifdef'ing this out, though.
 
    // Copy the arguments to avoid weird clobbery situations.
-   FrameTemp<char *> argPtrs (argc);
+   // [charliep - 02/06/2012; "clobbery situations" are handled now.]
+   //FrameTemp<char *> argPtrs (argc);
    
    U32 strdupWatermark = FrameAllocator::getWaterMark();
-   for( S32 i = 0; i < argc; i++ )
-   {
-      argPtrs[i] = reinterpret_cast<char *>( FrameAllocator::alloc( dStrlen( argv[i] ) + 1 ) );
-      dStrcpy( argPtrs[i], argv[i] );
-   }
 
    for( SimSet::iterator i = mBehaviors.begin(); i != mBehaviors.end(); i++ )
    {
@@ -144,20 +140,19 @@
       Namespace::Entry *pNSEntry = pNamespace->lookup(cbName);
       if( pNSEntry )
       {
-         // Set %this to our BehaviorInstance's Object ID
-         argPtrs[1] = const_cast<char *>( pBehavior->getIdString() );
-
          // [neo, 5/10/2007 - #3010]
          // Set flag so we can call the method on this object directly, should the
          // behavior have 'overloaded' a method on the owner object.
          mInBehaviorCallback = true;
 
          // Change the Current Console object, execute, restore Object
+         const char* saveArgv1 = argv[1];
+         argv[1] = pBehavior->getIdString();
          SimObject *save = gEvalState.thisObject;
          gEvalState.thisObject = pBehavior;
-         const char *ret = pNSEntry->execute(argc, const_cast<const char **>( ~argPtrs ), &gEvalState);
+         const char *ret = pNSEntry->execute(argc, argv, &gEvalState);
          gEvalState.thisObject = save;
-
+         argv[1] = saveArgv1;
          // [neo, 5/10/2007 - #3010]
          // Reset flag
          mInBehaviorCallback = false;
Index: console/compiledEval.cc
===================================================================
--- console/compiledEval.cc	(revision 192)
+++ console/compiledEval.cc	(working copy)
@@ -453,9 +453,6 @@
    const S32 nsDocLength = 128;
    char nsDocBlockClass[nsDocLength];
 
-   U32 callArgc;
-   const char **callArgv;
-
    static char curFieldArray[256];
    static char prevFieldArray[256];
 
@@ -531,6 +528,8 @@
             }
 
             // Get the constructor information off the stack.
+            U32 callArgc;
+            StringStack::ArgVBuffer callArgv;
             STR.getArgcArgv(NULL, &callArgc, &callArgv, true);
 
             // Con::printf("Creating object...");
@@ -1348,6 +1347,9 @@
             U32 callType = code[ip+2];
 
             ip += 3;
+
+            U32 callArgc;
+            StringStack::ArgVBuffer callArgv;
             STR.getArgcArgv(fnName, &callArgc, &callArgv);
 
             if(callType == FuncCallExprNode::FunctionCall) 
Index: console/stringStack.cc
===================================================================
--- console/stringStack.cc	(revision 192)
+++ console/stringStack.cc	(working copy)
@@ -5,19 +5,19 @@
 
 #include "console/stringStack.h"
 
-void StringStack::getArgcArgv(StringTableEntry name, U32 *argc, const char ***in_argv, bool popStackFrame /* = false */)
+void StringStack::getArgcArgv(StringTableEntry name, U32 *argc, StringStack::ArgVBuffer *in_argv, bool popStackFrame /* = false */)
 {
+   // note: we're passing in_argv as a reference b/c it feels more natural from the caller. (wasn't necessary)
+
    U32 startStack = mFrameOffsets[mNumFrames-1] + 1;
    U32 argCount   = getMin(mStartStackSize - startStack, (U32)MaxArgs);
 
-   *in_argv = mArgV;
-   mArgV[0] = name;
+   (*in_argv)[0] = name;
    
    for(U32 i = 0; i < argCount; i++)
-      mArgV[i+1] = mBuffer + mStartOffsets[startStack + i];
-   argCount++;
+      (*in_argv)[i+1] = mBuffer + mStartOffsets[startStack + i];
    
-   *argc = argCount;
+   *argc = argCount + 1;
 
    if(popStackFrame)
       popFrame();
Index: console/stringStack.h
===================================================================
--- console/stringStack.h	(revision 192)
+++ console/stringStack.h	(working copy)
@@ -22,9 +22,14 @@
       MaxArgs = 20,
       ReturnBufferSpace = 512
    };
+
+   // ArgVBuffer is an array of pointers similar to argv.
+   // Create one if you need to see the top frame of the StringStack as argv params.
+   // It is filled with pointers into a StringStack's buffer, so it must be const!
+   typedef const char *ArgVBuffer[MaxArgs];
+
    char *mBuffer;
    U32   mBufferSize;
-   const char *mArgV[MaxArgs];
    U32 mFrameOffsets[MaxStackDepth];
    U32 mStartOffsets[MaxStackDepth];
 
@@ -248,7 +253,7 @@
    }
 
    /// Get the arguments for a function call from the stack.
-   void getArgcArgv(StringTableEntry name, U32 *argc, const char ***in_argv, bool popStackFrame = false);
+   void getArgcArgv(StringTableEntry name, U32 *argc, ArgVBuffer *in_argv, bool popStackFrame = false);
 };
 
 #endif

Caveats!
Not sure there are any, but your code may have been counting on brokeness somehow, and this will unbreak it! I did not experience any negative effects with over 100 behaviors. I also was able to remove an "echo" I had added to an "onLevelLoaded()" just to avoid crashes until I patched it.