[BUG] Buffer overflow in console return buffer
by James Urquhart · in iTorque 2D · 02/27/2012 (11:33 am) · 2 replies
I'm not sure who wrote this. Maybe a monkey on holiday.
Basically for some reason the author has decided to return a static buffer(returnBuffer2) from Namespace::Entry::execute, even though the buffer from exec will probably suffice as that is generated from the StringStack.
What makes this problem 100 times worse is that they've chosen a minuscule return buffer size of 32 bytes, which is ok unless you want to store any useful data in a return string.
But that is not enough, no. In a final act of trying to give this code a purpose the author has taken it upon themselves to use dStrcpy in order to copy the potentially unlimited return buffer into a measly 32 bytes.
I really have no more words to explain how stupid this is.
For reference:
Basically for some reason the author has decided to return a static buffer(returnBuffer2) from Namespace::Entry::execute, even though the buffer from exec will probably suffice as that is generated from the StringStack.
What makes this problem 100 times worse is that they've chosen a minuscule return buffer size of 32 bytes, which is ok unless you want to store any useful data in a return string.
But that is not enough, no. In a final act of trying to give this code a purpose the author has taken it upon themselves to use dStrcpy in order to copy the potentially unlimited return buffer into a measly 32 bytes.
I really have no more words to explain how stupid this is.
For reference:
const char *Namespace::Entry::execute(S32 argc, const char **argv, ExprEvalState *state)
{
if(mType == ScriptFunctionType)
{
if(mFunctionOffset) {
recursionCount++;
//pass in recursionCount, the number of times this entry has been called
const char * ret = mCode->exec(mFunctionOffset, argv[0], mNamespace, argc, argv, false, mPackage, recursionCount );
recursionCount--;
//Luma: We need to return the return value here, but it must be in a static variable to remain valid
static char returnBuffer2[32]; // <=== 0____0;
dStrcpy(returnBuffer2, ret); // <=== x_x
return returnBuffer2;
}
else
return "";
}
About the author
#2
My point about ::exec returning a string allocated on the StringBuffer remains though. Looking at the evaluator it seems it trashes the stack then places the result back on, which would explain why one might want to keep the result around elsewhere. But to me this seems like a really bad design decision. What makes this even more bizarre is that ::getReturnBuffer seems to alternate between a dedicated buffer and the main stack. If it just used mArgBuffer (which oddly seems ONLY used by getReturnBuffer) instead there probably wouldn't be much of a problem.
In short i am tempted to just nuke this whole stack code and start from fresh.
02/27/2012 (3:13 pm)
Just noticed a duplicate report of this, http://www.garagegames.com/community/forums/viewthread/119817 - seems it was fixed in 1.4.1 but i seem to have missed it in my copy(!).My point about ::exec returning a string allocated on the StringBuffer remains though. Looking at the evaluator it seems it trashes the stack then places the result back on, which would explain why one might want to keep the result around elsewhere. But to me this seems like a really bad design decision. What makes this even more bizarre is that ::getReturnBuffer seems to alternate between a dedicated buffer and the main stack. If it just used mArgBuffer (which oddly seems ONLY used by getReturnBuffer) instead there probably wouldn't be much of a problem.
In short i am tempted to just nuke this whole stack code and start from fresh.
Torque Owner Nathan Martin
TRON 2001 Network