Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible memory corruption with VariableDeltaSerializer #41

Open
Snalibe opened this issue Jun 21, 2017 · 0 comments
Open

Possible memory corruption with VariableDeltaSerializer #41

Snalibe opened this issue Jun 21, 2017 · 0 comments

Comments

@Snalibe
Copy link

Snalibe commented Jun 21, 2017

I'm just dropping this here so that the community knows...

I found an issue when serializing too many variables using the variabledeltaserializer.

bool WriteVarToBitstream(const VarType &varData, RakNet::BitStream *bitStream, unsigned char *bArray, unsigned short writeOffset)
{
	static bool checkHeap = false;

	if (WriteVarToBitstream(varData,bitStream)==true)
	{
		BitSize_t numberOfBitsMod8 = writeOffset & 7;

		if (numberOfBitsMod8 == 0)
			---------> bArray[writeOffset >> 3] = 0x80; <-----------
		else
			---------> bArray[writeOffset >> 3] |= 0x80 >> (numberOfBitsMod8); // Set the bit to 1 <-----------

These lines will cause memory overflow if writeOffset exceeds 447.

This is due to the bitField this class uses:

struct ChangedVariablesList
{
	uint32_t sendReceipt;
	unsigned short bitWriteIndex;
	unsigned char bitField[------------>56<-----------];
};

By increasing this value, we can extend the maximum number of variables that can be serialized. But this should be made dynamic.

More importantly, a hard crash should be put in place when this is detected and an assert explaining what's the problem. It would have saved me a few days of debugging!

Here is my guard (not a fix per say):

template <class VarType>
void SerializeVariable(SerializationContext *context, const VarType &variable)
{
	context->variableHistory->variableListDeltaTracker.fCheckHeap = fCheckHeap;

	if (context->newSystemSend)
	{
		if (context->variableHistory->variableListDeltaTracker.IsPastEndOfList()==false)
		{
			// previously sent data to another system
			context->bitStream->Write(true);
			context->bitStream->Write(variable);
			context->anyVariablesWritten=true;
		}
		else
		{
			// never sent data to another system
			context->variableHistory->variableListDeltaTracker.WriteVarToBitstream(variable, context->bitStream);
			context->anyVariablesWritten=true;
		}
	}
	else if (context->serializationMode==UNRELIABLE_WITH_ACK_RECEIPT)
	{
		------------>RakAssert((context->changedVariables->bitWriteIndex >> 3) < 56);

		------------>if (!((context->changedVariables->bitWriteIndex >> 3) < 56))
		------------>{
		------------>	// Crash here. Continuing execution will lead to memory corruption.
		------------>	// What does this mean? Too many variables to serialize. Need to split them into more replicas or increase the bitField size.
		------------>	*(int*)123 = 123;
		------------>}

		context->anyVariablesWritten|=
		context->variableHistory->variableListDeltaTracker.WriteVarToBitstream(variable, context->bitStream, context->changedVariables->bitField, context->changedVariables->bitWriteIndex++);
	}
	else
	{
		if (context->variableHistoryIdentical)
		{
			// Identical serialization to a number of systems
			if (didComparisonThisTick==false)
				context->anyVariablesWritten|=
				context->variableHistory->variableListDeltaTracker.WriteVarToBitstream(variable, context->bitStream);
			// Else bitstream is written to at the end
		}
		else
		{
			// Per-system serialization
			context->anyVariablesWritten|=
				context->variableHistory->variableListDeltaTracker.WriteVarToBitstream(variable, context->bitStream);
		}
	}
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant