larku / RakNet

RakNet is a cross platform, open source, C++ networking engine for game programmers.
Other
60 stars 27 forks source link

Possible memory corruption with VariableDeltaSerializer #41

Open Snalibe opened 7 years ago

Snalibe commented 7 years ago

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