Inspector Dynamic Field bug in TGE 1.5?

Quote:Edit: darn, you know I meant "Inpsector" dynamic fields bug

void GuiInspectorDynamicField::renameField( StringTableEntry newFieldName )
{
   if( mTarget == NULL || mDynField == NULL || mParent == NULL || mEdit == NULL )
   {
      Con::warnf("GuiInspectorDynamicField::renameField - No target object or dynamic field data found!" );
      return;
   }

   StringTableEntry name = StringTable->insert(newFieldName);


   if( !name )
   {
      Con::warnf("GuiInspectorDynamicField::renameField - Invalid field name specified!" );
      return;
   }

   // Only proceed if the name has changed
   if( dStricmp( name, getFieldName() ) == 0 )
      return;

   // Grab a pointer to our parent and cast it to GuiInspectorDynamicGroup
   GuiInspectorDynamicGroup *group = dynamic_cast<GuiInspectorDynamicGroup*>(mParent);

   if( group == NULL )
   {
      Con::warnf("GuiInspectorDynamicField::renameField - Unable to locate GuiInspectorDynamicGroup parent!" );
      return;
   }

   // Grab our current dynamic field value
   StringTableEntry currentValue = getData();

   // Create our new field with the value of our old field and the new fields name!
   mTarget->setDataField( name, NULL, currentValue );

   // Configure our field to grab data from the new dynamic field
   SimFieldDictionary::Entry *newEntry = group->findDynamicFieldInDictionary( name );

   if( newEntry == NULL )
   {
      Con::warnf("GuiInspectorDynamicField::renameField - Unable to find new field!" );
      return;
   }

   // Set our old fields data to "" (which will effectively erase the field)
   mTarget->setDataField( getFieldName(), NULL, "" );
   
   // Assign our dynamic field pointer (where we retrieve field information from) to our new field pointer
   mDynField = newEntry;

   // Reassign the caption of the field (to match the new field name)
   // Note : we use the getFieldName accessor which, since we have changed our field pointer
   //        will grab the slotName from the new field
   mCaption = getFieldName();

   // Lastly we need to reassign our Command and AltCommand fields for our value edit control
   char szBuffer[512];
   dSprintf( szBuffer, 512, "%d.%s = %d.getText();",mTarget->getId(), getFieldName(), mEdit->getId() );
   mEdit->setField("AltCommand", szBuffer );
   mEdit->setField("Validate", szBuffer );
}

Quote:

I second that. This is one of the worst bugs I've seen in a while, purely because it's so easy for anyone to do it, and it's results are incredibly obscure and hard to track down.

This bug has actually been annoying me since a long time before it was brought up on the forums, but I didn't have time to put in the significant amount of effort it took to figure out a solid repro case. Once Matt had done that hard work, the fix was relatively obvious within a few seconds debugging ;-)

In case you are wondering, here's the gory details:

The value that was being passed to the setData() method came from a ConsoleMethod's argv array. The script interpreter uses a static array for argv for calls made from script.

The setData() method calls inspectPreApply(). For GuiControls, this calls onSleep(), which in turn causes a call into script to the script version of onSleep(). At this point, everything is still fine since the call to Con::executef() specified a new argv array.

However, if the script onSleep() calls another function or method (in script), then the script interpreter's argv array will get munged. Since the value passed to setData() was part of that argv array, the data gets munged.

Thus, using the StringTable means we get a nice static string that won't get munged. We could also have copied it into a buffer, but that would be lame since the StringTable is there ;-)

One thing to note about this fix is that since setData() takes a StringTableEntry, it's arguable that it expects it's arg to already be in the string table and thus the proper fix would be to fix the apply() ConsoleMethod. However, I figured it was safer to fix it this way just in case there is other code that calls it without a string table entry. In any case, inserting something to the string table that's already in there returns the same pointer (which is part of the point in it's existence anyway) and it's not really an area of the code where we need to worry about performance.

The moral of this story? If you have some data that may have come through args to a ConsoleFunction / ConsoleMethod and you call into script, you'd better make sure that you have a safe version of it if you want to use that data after the Con::executef(). You're in for a whole world of hurt otherwise ;-)