Merge pull request #2233 from CouleeApps/better-return-buffer

Use a circular buffer for getReturnBuffer to prevent buffer cloberring

Engine/source/console/compiledEval.cpp

@@ -41,6 +41,7 @@
 #include "core/frameAllocator.h"
 
 #include "console/codeInterpreter.h"
+#include "console/returnBuffer.h"
 
 #ifndef TORQUE_TGB_ONLY
 #include "materials/materialDefinition.h"
@@ -101,17 +102,17 @@ F64 consoleStringToNumber(const char *str, StringTableEntry file, U32 line)
 
 namespace Con
 {
+   ReturnBuffer retBuffer;
 
    char *getReturnBuffer(U32 bufferSize)
-
    {
-      return STR.getReturnBuffer(bufferSize);
+      return retBuffer.getBuffer(bufferSize);
    }
 
    char *getReturnBuffer(const char *stringToCopy)
    {
       U32 len = dStrlen(stringToCopy) + 1;
-      char *ret = STR.getReturnBuffer(len);
+      char *ret = retBuffer.getBuffer(len);
       dMemcpy(ret, stringToCopy, len);
       return ret;
    }
@@ -119,7 +120,7 @@ namespace Con
    char* getReturnBuffer(const String& str)
    {
       const U32 size = str.size();
-      char* ret = STR.getReturnBuffer(size);
+      char* ret = retBuffer.getBuffer(size);
       dMemcpy(ret, str.c_str(), size);
       return ret;
    }

Engine/source/console/returnBuffer.cpp

@@ -0,0 +1,83 @@
+//-----------------------------------------------------------------------------
+// Copyright (c) 2012 GarageGames, LLC
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to
+// deal in the Software without restriction, including without limitation the
+// rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+// sell copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+// IN THE SOFTWARE.
+//-----------------------------------------------------------------------------
+
+#include "returnBuffer.h"
+
+ReturnBuffer::ReturnBuffer()
+{
+   mBuffer = nullptr;
+   mBufferSize = 0;
+   mStart = 0;
+
+   //Decent starting alloc of ~1 page = 4kb
+   ensureSize(4 * 1024);
+}
+
+ReturnBuffer::~ReturnBuffer()
+{
+   if (mBuffer)
+   {
+      dFree(mBuffer);
+   }
+}
+
+void ReturnBuffer::ensureSize(U32 newSize)
+{
+   //Round up to nearest multiple of 16 bytes
+   if (newSize & 0xF)
+   {
+      newSize = (newSize & ~0xF) + 0x10;
+   }
+   if (mBuffer == NULL)
+   {
+      //First alloc
+      mBuffer = (char *)dMalloc(newSize * sizeof(char));
+      mBufferSize = newSize;
+   }
+   else if (mBufferSize < newSize)
+   {
+      //Just use the expected size
+      mBuffer = (char *)dRealloc(mBuffer, newSize * sizeof(char));
+      mBufferSize = newSize;
+   }
+}
+
+char *ReturnBuffer::getBuffer(U32 size, U32 alignment)
+{
+   ensureSize(size);
+
+   //Align the start if necessary
+   if (mStart % alignment != 0)
+   {
+      mStart += alignment - (mStart % alignment);
+   }
+
+   if (size + mStart > mBufferSize)
+   {
+      //Restart
+      mStart = 0;
+   }
+   char *buffer = mBuffer + mStart;
+   mStart += size;
+
+   return buffer;
+}

Engine/source/console/returnBuffer.h

@@ -0,0 +1,49 @@
+//-----------------------------------------------------------------------------
+// Copyright (c) 2012 GarageGames, LLC
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to
+// deal in the Software without restriction, including without limitation the
+// rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+// sell copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+// IN THE SOFTWARE.
+//-----------------------------------------------------------------------------
+
+#ifndef _RETURNBUFFER_H_
+#define _RETURNBUFFER_H_
+
+#ifndef _PLATFORM_H_
+   #include <platform/platform.h>
+#endif
+
+/// Simple circular buffer class for temporary return storage.
+class ReturnBuffer
+{
+   char *mBuffer;
+   U32 mBufferSize;
+   U32 mStart;
+
+   /// Possibly expand the buffer to be larger than newSize
+   void ensureSize(U32 newSize);
+
+public:
+   ReturnBuffer();
+   ~ReturnBuffer();
+
+   /// Get a temporary buffer with a given size (and alignment)
+   /// @note The buffer will be re-used so do not consider it permanent
+   char *getBuffer(U32 size, U32 alignment = 16);
+};
+
+#endif //_RETURNBUFFER_H_

Engine/source/console/stringStack.cpp

@@ -24,6 +24,185 @@
 #include "console/consoleInternal.h"
 #include "console/stringStack.h"
 
+StringStack::StringStack()
+{
+   mBufferSize = 0;
+   mBuffer = NULL;
+   mArgBufferSize = 0;
+   mArgBuffer = NULL;
+   mNumFrames = 0;
+   mStart = 0;
+   mLen = 0;
+   mStartStackSize = 0;
+   mFunctionOffset = 0;
+   validateBufferSize(8192);
+   validateArgBufferSize(2048);
+   dMemset(mBuffer, '\0', mBufferSize);
+   dMemset(mArgBuffer, '\0', mArgBufferSize);
+}
+
+StringStack::~StringStack()
+{
+   if( mBuffer )
+      dFree( mBuffer );
+   if( mArgBuffer )
+      dFree( mArgBuffer );
+}
+
+void StringStack::validateBufferSize(U32 size)
+{
+   if(size > mBufferSize)
+   {
+      mBufferSize = size + 2048;
+      mBuffer = (char *) dRealloc(mBuffer, mBufferSize);
+   }
+}
+
+void StringStack::validateArgBufferSize(U32 size)
+{
+   if(size > mArgBufferSize)
+   {
+      mArgBufferSize = size + 2048;
+      mArgBuffer = (char *) dRealloc(mArgBuffer, mArgBufferSize);
+   }
+}
+
+void StringStack::setIntValue(U32 i)
+{
+   validateBufferSize(mStart + 32);
+   dSprintf(mBuffer + mStart, 32, "%d", i);
+   mLen = dStrlen(mBuffer + mStart);
+}
+
+void StringStack::setFloatValue(F64 v)
+{
+   validateBufferSize(mStart + 32);
+   dSprintf(mBuffer + mStart, 32, "%g", v);
+   mLen = dStrlen(mBuffer + mStart);
+}
+
+char *StringStack::getReturnBuffer(U32 size)
+{
+   if(size > ReturnBufferSpace)
+   {
+      AssertFatal(Con::isMainThread(), "Manipulating return buffer from a secondary thread!");
+      validateArgBufferSize(size);
+      return mArgBuffer;
+   }
+   else
+   {
+      validateBufferSize(mStart + size);
+      return mBuffer + mStart;
+   }
+}
+
+char *StringStack::getArgBuffer(U32 size)
+{
+   AssertFatal(Con::isMainThread(), "Manipulating console arg buffer from a secondary thread!");
+   validateBufferSize(mStart + mFunctionOffset + size);
+   char *ret = mBuffer + mStart + mFunctionOffset;
+   mFunctionOffset += size;
+   return ret;
+}
+
+void StringStack::clearFunctionOffset()
+{
+   //Con::printf("StringStack mFunctionOffset = 0 (from %i)", mFunctionOffset);
+   mFunctionOffset = 0;
+}
+
+void StringStack::setStringValue(const char *s)
+{
+   if(!s)
+   {
+      mLen = 0;
+      mBuffer[mStart] = 0;
+      return;
+   }
+   mLen = dStrlen(s);
+
+   validateBufferSize(mStart + mLen + 2);
+   dStrcpy(mBuffer + mStart, s, mBufferSize - mStart);
+}
+
+void StringStack::advance()
+{
+   mStartOffsets[mStartStackSize++] = mStart;
+   mStart += mLen;
+   mLen = 0;
+}
+
+void StringStack::advanceChar(char c)
+{
+   mStartOffsets[mStartStackSize++] = mStart;
+   mStart += mLen;
+   mBuffer[mStart] = c;
+   mBuffer[mStart+1] = 0;
+   mStart += 1;
+   mLen = 0;
+}
+
+void StringStack::push()
+{
+   advanceChar(0);
+}
+
+void StringStack::rewind()
+{
+   mStart = mStartOffsets[--mStartStackSize];
+   mLen = dStrlen(mBuffer + mStart);
+}
+
+void StringStack::rewindTerminate()
+{
+   mBuffer[mStart] = 0;
+   mStart = mStartOffsets[--mStartStackSize];
+   mLen   = dStrlen(mBuffer + mStart);
+}
+
+U32 StringStack::compare()
+{
+   // Figure out the 1st and 2nd item offsets.
+   U32 oldStart = mStart;
+   mStart = mStartOffsets[--mStartStackSize];
+
+   // Compare current and previous strings.
+   U32 ret = !dStricmp(mBuffer + mStart, mBuffer + oldStart);
+
+   // Put an empty string on the top of the stack.
+   mLen = 0;
+   mBuffer[mStart] = 0;
+
+   return ret;
+}
+
+void StringStack::pushFrame()
+{
+   //Con::printf("StringStack pushFrame [frame=%i, start=%i]", mNumFrames, mStartStackSize);
+   mFrameOffsets[mNumFrames++] = mStartStackSize;
+   mStartOffsets[mStartStackSize++] = mStart;
+   mStart += ReturnBufferSpace;
+   validateBufferSize(0);
+}
+
+void StringStack::popFrame()
+{
+   //Con::printf("StringStack popFrame [frame=%i, start=%i]", mNumFrames, mStartStackSize);
+   mStartStackSize = mFrameOffsets[--mNumFrames];
+   mStart = mStartOffsets[mStartStackSize];
+   mLen = 0;
+}
+
+void StringStack::clearFrames()
+{
+   //Con::printf("StringStack clearFrames");
+   mNumFrames = 0;
+   mStart = 0;
+   mLen = 0;
+   mStartStackSize = 0;
+   mFunctionOffset = 0;
+}
+
 
 void ConsoleValueStack::getArgcArgv(StringTableEntry name, U32 *argc, ConsoleValueRef **in_argv, bool popStackFrame /* = false */)
 {

Engine/source/console/stringStack.h

@@ -79,105 +79,31 @@ struct StringStack
    U32 mArgBufferSize;
    char *mArgBuffer;
 
-   void validateBufferSize(U32 size)
-   {
-      if(size > mBufferSize)
-      {
-         mBufferSize = size + 2048;
-         mBuffer = (char *) dRealloc(mBuffer, mBufferSize);
-      }
-   }
-
-   void validateArgBufferSize(U32 size)
-   {
-      if(size > mArgBufferSize)
-      {
-         mArgBufferSize = size + 2048;
-         mArgBuffer = (char *) dRealloc(mArgBuffer, mArgBufferSize);
-      }
-   }
+   void validateBufferSize(U32 size);
+   void validateArgBufferSize(U32 size);
 
-   StringStack()
-   {
-      mBufferSize = 0;
-      mBuffer = NULL;
-      mArgBufferSize = 0;
-      mArgBuffer = NULL;
-      mNumFrames = 0;
-      mStart = 0;
-      mLen = 0;
-      mStartStackSize = 0;
-      mFunctionOffset = 0;
-      validateBufferSize(8192);
-      validateArgBufferSize(2048);
-      dMemset(mBuffer, '\0', mBufferSize);
-      dMemset(mArgBuffer, '\0', mArgBufferSize);
-   }
-   ~StringStack()
-   {
-      if( mBuffer )
-         dFree( mBuffer );
-      if( mArgBuffer )
-         dFree( mArgBuffer );
-   }
+   StringStack();
+   ~StringStack();
 
    /// Set the top of the stack to be an integer value.
-   void setIntValue(U32 i)
-   {
-      validateBufferSize(mStart + 32);
-      dSprintf(mBuffer + mStart, 32, "%d", i);
-      mLen = dStrlen(mBuffer + mStart);
-   }
+   void setIntValue(U32 i);
 
    /// Set the top of the stack to be a float value.
-   void setFloatValue(F64 v)
-   {
-      validateBufferSize(mStart + 32);
-      dSprintf(mBuffer + mStart, 32, "%g", v);
-      mLen = dStrlen(mBuffer + mStart);
-   }
+   void setFloatValue(F64 v);
 
    /// Return a temporary buffer we can use to return data.
-   char* getReturnBuffer(U32 size)
-   {
-      AssertFatal(Con::isMainThread(), "Manipulating return buffer from a secondary thread!");
-      validateArgBufferSize(size);
-      return mArgBuffer;
-   }
+   char* getReturnBuffer(U32 size);
 
    /// Return a buffer we can use for arguments.
    ///
    /// This updates the function offset.
-   char *getArgBuffer(U32 size)
-   {
-      AssertFatal(Con::isMainThread(), "Manipulating console arg buffer from a secondary thread!");
-      validateBufferSize(mStart + mFunctionOffset + size);
-      char *ret = mBuffer + mStart + mFunctionOffset;
-      mFunctionOffset += size;
-      return ret;
-   }
+   char *getArgBuffer(U32 size);
 
    /// Clear the function offset.
-   void clearFunctionOffset()
-   {
-      //Con::printf("StringStack mFunctionOffset = 0 (from %i)", mFunctionOffset);
-      mFunctionOffset = 0;
-   }
+   void clearFunctionOffset();
 
    /// Set a string value on the top of the stack.
-   void setStringValue(const char *s)
-   {
-      if(!s)
-      {
-         mLen = 0;
-         mBuffer[mStart] = 0;
-         return;
-      }
-      mLen = dStrlen(s);
-
-      validateBufferSize(mStart + mLen + 2);
-      dStrcpy(mBuffer + mStart, s, mBufferSize - mStart);
-   }
+   void setStringValue(const char *s);
 
    /// Get the top of the stack, as a StringTableEntry.
    ///
@@ -226,33 +152,17 @@ struct StringStack
    ///
    /// @note You should use StringStack::push, not this, if you want to
    ///       properly push the stack.
-   void advance()
-   {
-      mStartOffsets[mStartStackSize++] = mStart;
-      mStart += mLen;
-      mLen = 0;
-   }
+   void advance();
 
    /// Advance the start stack, placing a single character, null-terminated strong
    /// on the top.
    ///
    /// @note You should use StringStack::push, not this, if you want to
    ///       properly push the stack.
-   void advanceChar(char c)
-   {
-      mStartOffsets[mStartStackSize++] = mStart;
-      mStart += mLen;
-      mBuffer[mStart] = c;
-      mBuffer[mStart+1] = 0;
-      mStart += 1;
-      mLen = 0;
-   }
+   void advanceChar(char c);
 
    /// Push the stack, placing a zero-length string on the top.
-   void push()
-   {
-      advanceChar(0);
-   }
+   void push();
 
    inline void setLen(U32 newlen)
    {
@@ -260,64 +170,20 @@ struct StringStack
    }
 
    /// Pop the start stack.
-   void rewind()
-   {
-      mStart = mStartOffsets[--mStartStackSize];
-      mLen = dStrlen(mBuffer + mStart);
-   }
+   void rewind();
 
    // Terminate the current string, and pop the start stack.
-   void rewindTerminate()
-   {
-      mBuffer[mStart] = 0;
-      mStart = mStartOffsets[--mStartStackSize];
-      mLen   = dStrlen(mBuffer + mStart);
-   }
+   void rewindTerminate();
 
    /// Compare 1st and 2nd items on stack, consuming them in the process,
    /// and returning true if they matched, false if they didn't.
-   U32 compare()
-   {
-      // Figure out the 1st and 2nd item offsets.
-      U32 oldStart = mStart;
-      mStart = mStartOffsets[--mStartStackSize];
-
-      // Compare current and previous strings.
-      U32 ret = !dStricmp(mBuffer + mStart, mBuffer + oldStart);
-
-      // Put an empty string on the top of the stack.
-      mLen = 0;
-      mBuffer[mStart] = 0;
+   U32 compare();
 
-      return ret;
-   }
-
-   void pushFrame()
-   {
-      //Con::printf("StringStack pushFrame [frame=%i, start=%i]", mNumFrames, mStartStackSize);
-      mFrameOffsets[mNumFrames++] = mStartStackSize;
-      mStartOffsets[mStartStackSize++] = mStart;
-      mStart += ReturnBufferSpace;
-      validateBufferSize(0);
-   }
+   void pushFrame();
 
-   void popFrame()
-   {
-      //Con::printf("StringStack popFrame [frame=%i, start=%i]", mNumFrames, mStartStackSize);
-      mStartStackSize = mFrameOffsets[--mNumFrames];
-      mStart = mStartOffsets[mStartStackSize];
-      mLen = 0;
-   }
+   void popFrame();
 
-   void clearFrames()
-   {
-      //Con::printf("StringStack clearFrames");
-      mNumFrames = 0;
-      mStart = 0;
-      mLen = 0;
-      mStartStackSize = 0;
-      mFunctionOffset = 0;
-   }
+   void clearFrames();
 
    /// Get the arguments for a function call from the stack.
    void getArgcArgv(StringTableEntry name, U32 *argc, const char ***in_argv, bool popStackFrame = false);