T3D 1.1 Beta 2 - EngineMarshallData memory overwrite - LOGGED
by Yuri Dobronravin · in Torque 3D Professional · 09/14/2010 (8:10 pm) · 3 replies
Build: 1.1 Beta 2
Platform: Windows XP, 32 bit
Target: EngineMarshallData, working with console functions arguments
Issues:
When parsing a multi-argument functions with EngineMarshallData memory allocations done in a wrong way resulting in a broken argument stack passed to script function.
Steps to Repeat:
Found this bug in Projectile::onCollision, while calling script callback mDataBlock->onCollision_callback.
But it appears to be more general issue.
Fire with projectile into some obstacle and put a break point to Projectile::onCollision.
Step in with F11 till you get here:
Code with my comments:
Suggested Fix:
Changing getReturnBuffer to getArgBuffer worked for me but general consequences should be evaluated and analyzed.
mathTypes.cpp
Platform: Windows XP, 32 bit
Target: EngineMarshallData, working with console functions arguments
Issues:
When parsing a multi-argument functions with EngineMarshallData memory allocations done in a wrong way resulting in a broken argument stack passed to script function.
Steps to Repeat:
Found this bug in Projectile::onCollision, while calling script callback mDataBlock->onCollision_callback.
But it appears to be more general issue.
Fire with projectile into some obstacle and put a break point to Projectile::onCollision.
Step in with F11 till you get here:
template< typename R, typename A, typename B, typename C, typename D, typename E >
R call( A a, B b, C c, D d, E e )
{
EngineMarshallData( a, mArgc, mArgv );
EngineMarshallData( b, mArgc, mArgv );
EngineMarshallData( c, mArgc, mArgv );
EngineMarshallData( d, mArgc, mArgv );
EngineMarshallData( e, mArgc, mArgv );
return R( EngineUnmarshallData< R >()( _exec() ) );
}And look at how mArgv array is changed. On the step 4 - EngineMarshallData( d, mArgc, mArgv ) - mArgv array become corrupted for the reason of the bug that code in generic EngineMarshallData template contains. Code with my comments:
/// Marshal data from native into client form stored directly in
/// client function invocation vector.
template< typename T >
inline void EngineMarshallData( const T& arg, S32& argc, const char** argv )
{
//!!!!!! My Comments !!!!
// Following line made a call to ConsoleGetType( TypePoint3F ) which in place
// calls Con::getReturnBuffer(256). getReturnBuffer - refers to the same buffer
// that used in Con::getArgBuffer but without mFunctionOffset. As result previous call to
// EngineMarshallData would be overwritten.
const char* tmpRes = castConsoleTypeToString( arg );
U32 size = dStrlen( tmpRes ) + 1;
char* tmp = ( char* ) Con::getArgBuffer( size );
dMemcpy( tmp, tmpRes, size );
argv[ argc ] = tmp;
argc ++;
}Suggested Fix:
Changing getReturnBuffer to getArgBuffer worked for me but general consequences should be evaluated and analyzed.
mathTypes.cpp
ConsoleGetType( TypePoint3F )
{
Point3F *pt = (Point3F *) dptr;
//char* returnBuffer = Con::getReturnBuffer(256);
char* returnBuffer = Con::getArgBuffer(256);
dSprintf(returnBuffer, 256, "%g %g %g", pt->x, pt->y, pt->z);
return returnBuffer;
}
#2
The bug is in StringStack. The problem is that the first time you call getArgBuffer, both mStart & mFunctionOffset are zero. This means the first call got getArgBuffer will always return the same pointer as getReturnBuffer does.
As you make subsequent calls into the string stack using return buffers to convert parameters into strings, each of them that uses getReturnBuffer will overwrite whatever is in the first argument buffer (as it's the same pointer as the return buffer).
This is a pretty big bug, as it means any IMPLEMENT_CALLBACK function that uses more than 1 parameter that uses the string stack to convert it's parameters (all of Point, Matrix, etc parameters) the first argument on the stack is going to be corrupted.
On cursory examination, it seems that if we just allocated the first (whatever feels comfortable, say 512 bytes) to the return buffer, and initialize mFunctionOffset to this value to begin with, it should be fine. But I second Yuri's suggestion that general consequences should be considered and analyzed.
Dusty
10/18/2010 (10:18 pm)
I just spent this afternoon tracking down this bug in my own code, so I'm glad to see it's listed here. Here's some additional information:The bug is in StringStack. The problem is that the first time you call getArgBuffer, both mStart & mFunctionOffset are zero. This means the first call got getArgBuffer will always return the same pointer as getReturnBuffer does.
As you make subsequent calls into the string stack using return buffers to convert parameters into strings, each of them that uses getReturnBuffer will overwrite whatever is in the first argument buffer (as it's the same pointer as the return buffer).
This is a pretty big bug, as it means any IMPLEMENT_CALLBACK function that uses more than 1 parameter that uses the string stack to convert it's parameters (all of Point, Matrix, etc parameters) the first argument on the stack is going to be corrupted.
On cursory examination, it seems that if we just allocated the first (whatever feels comfortable, say 512 bytes) to the return buffer, and initialize mFunctionOffset to this value to begin with, it should be fine. But I second Yuri's suggestion that general consequences should be considered and analyzed.
Dusty
#3
I would have thought this one would have caused more trouble.
02/22/2011 (7:36 am)
Looks like a typo to me. My fix.char *getArgBuffer(U32 size)
{
validateBufferSize(mStart + mFunctionOffset + size);
// start jc
// char *ret = mBuffer + mStart + mFunctionOffset;
char *ret = mArgBuffer + mStart + mFunctionOffset;
// end jc
mFunctionOffset += size;
return ret;
}I would have thought this one would have caused more trouble.
Associate David Montgomery-Blake
David MontgomeryBlake