Game Development Community

Unlikely but present buffer overflow in expandEscape()

by Orion Elenzil · in Torque Game Engine · 10/24/2006 (5:31 pm) · 7 replies

There is a bug in the ConsoleFunction expandEscape() which one is unlikely to encounter, but it's not out of the question.

original function
ConsoleFunction(expandEscape, const char *, 2, 2, "expandEscape(text)")
{
   argc;
   char *ret = Con::getReturnBuffer(dStrlen(argv[1])*2 + 1);  // [b]worst case situation.[/b]
   expandEscape(ret, argv[1]);
   return ret;
}


however, strlen() * 2 + 1 is not the worst case.
the worst case occurs when your entire string consists of characters who's decimal value is less than 32.
in that case, every character will be replaced with four characters representing its hex value. eg: "\xFF".

fixed function
ConsoleFunction(expandEscape, const char *, 2, 2, "expandEscape(text)")
{
   argc;
   char *ret = Con::getReturnBuffer(dStrlen(argv[1])*4 + 1);  // [b]worst case situation.[/b]
   expandEscape(ret, argv[1]);
   return ret;
}


for more insight, check out expandEscape() in scan.cc.
down near the bottom is the case in question: else if(c < 32).


PS
i'm modifying expandEscape() itself to take an optional bufferSize parameter the way dSprintf() does.
let me know if there's interest.
this caused us to crash when we had fields of a SimObject which were longer than 1024 chars and the dump() method was called on the object.
NextNext Thread
PrevPrevious Thread
More from Torque Game Engine


Associate Orion Elenzil
Real Life Plus
#1
10/25/2006 (9:40 am)
Note - should add this is in the TGE 1.3 codebase; i haven't checked 1.4 etc,
and my example of "\xff" is of course impossible. ;) the values could only be in the range 0x01 to 0x1f.

Associate Orion Elenzil
Real Life Plus
#2
10/25/2006 (10:12 am)
Unlikely that anyone cares, but here is the improved version of expandEscape() which takes an optional bufSize param and won't overflow it. If you don't specify a bufSize, it will assume the bufSize is sufficient for the worst case, which is effectively the old behaviour.


engine/console.h
change this:
extern void expandEscape(char *dest, const char *src);
to this:
extern void expandEscape(char *dest, const char *src, S32 bufSize = -1);

engine/console/scan.cc
change this:
void expandEscape(char *dest, const char *src)
{
   unsigned char c;
   while((c = (unsigned char) *src++) != 0)
to this:
void expandEscape(char *dest, const char *src, S32 bufSize)
{
   unsigned char c;

   if (bufSize < 0)  // assume they know what they're doing.
      bufSize = dStrlen(src) * 4 + 1;

   char* destMax = dest + bufSize - 1;     // allow for NULL-terminator, plus "\xFF".

   while(((c = (unsigned char) *src++) != 0) && dest <= destMax - 4)

and change this:
*dest = '[[62810e2059004]]';
}
to this:
// always terminate.
   *dest = '[[62810e20590cf]]';

   if (c != 0)
      Con::errorf(ConsoleLogEntry::General, "Buffer overflow in expandEscape(). String truncated.");
}

Associate Orion Elenzil
Real Life Plus
#3
10/25/2006 (10:20 am)
For people who are into tests:
ConsoleFunction(testExpandEscape, const char *, 2, 2, "testExpandEscape(bool passOrFail)")
{
   char s[]     = {30, 31, '[[62810e205d24e]]'};

   S32  bufSize = dStrlen(s)*4 + 1;

   char *ret    = Con::getReturnBuffer(bufSize);

   if (dAtob(argv[1]))
   {
      // test with bufSize just right
      expandEscape(ret, s, bufSize    );
   }
   else
   {
      // test with bufSize too small by one byte.
      expandEscape(ret, s, bufSize - 1);
   }

   return ret;
}

Associate Orion Elenzil
Real Life Plus
#4
10/25/2006 (11:14 am)
Last post on this, i hope !

so the next part of this is reworking the calls to expandEscape() to provide bufferSize info,
which isn't hard, but does touch several files:

tge/engine/console/consoleFunctions.cc
tge/engine/console/consoleInternal.cc
tge/engine/console/simBase.cc
tge/engine/gui/guiInspector.cc
tge/engine/gui/messageVector.cc
tge/engine/sim/actionMap.cc
tge/engine/sim/connectionStringTable.cc

.. so if you're getting crashes related to long strings in any of those areas, this might be a suspect.

if anyone's interested in the full patch for those files, let me know.

Torque Owner Stefan Lundmark
#5
10/25/2006 (11:23 am)
Interesting changes there.

Associate Orion Elenzil
Real Life Plus
#6
10/25/2006 (12:09 pm)
One more note -
*if* you've got hyuge strings as members of an object and *if* you want to see the whole string as a result of dump(), then it would behyoove you to change SimFieldDictionary::printFields() and ConsoleMethod(SimObject,dump...) to dynamically grow expandedBuffer if/as needed.

again, it's simple stuff, but if anyone wants more details just ping me.

Torque Owner Duncan Gray
#7
10/25/2006 (2:35 pm)
Thanks for posting.