Skip to content

Possible memory corruption with VariableDeltaSerializer #41

@Snalibe

Description

@Snalibe

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);
		}
	}
}

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions