Packet Corruption on 64bit

[garrynewman]

I've been having issues where packets are apparently corrupted. This seems to only affect about 1 in 50 people, who it seems to happen to quite regularly. I've added CRCs to both datagrams and packet content, the CRC on the datagrams always pass. The CRC on the packet content itself fails very occasionally.

This apparently happens when packet size > MTU. Could there be an issue when sending lots of data to 100+ connected clients where the client would reconstruct packets wrong?

[garrynewman]

I did some logging and found that the issue is only apparent when the packets are split, and when the first fragment isn't received first.

It seems to be that split packets aren't being reconstructed in the correct order, but in the order in which the packets are received. I've changed the BuildPacketFromSplitPacketList function to construct the packets in order splitPacketIndex - will report whether this fixes it.

[larku]

Hi @garrynewman,

If you could contribute a patch/pull request that would be great - I'm using RakNet in an application that will often have packets > MTU so I'm very interested in any fixes here.

Regards.

[larku]

Hey @garrynewman

Did you have any success with your fixes?

Note, the latest source already appears to use splitPacketIndex when reconstructing the packets:

memcpy(internalPacket->data+splitPacket->splitPacketIndex*splitPacketPartLength, splitPacket->data, (size_t) BITS_TO_BYTES(splitPacketChannel->splitPacketList[j]->dataBitLength));

Regards.

[garrynewman]

Yeah my fix did fix it for me. Here's what I changed in BuildPacketFromSplitPacketList:

    for (j=0; j < splitPacketChannel->splitPacketList.Size(); j++)
    {
        splitPacket=splitPacketChannel->splitPacketList[j];
        memcpy(internalPacket->data + BITS_TO_BYTES(offset), splitPacket->data, (size_t)BITS_TO_BYTES(splitPacketChannel->splitPacketList[j]->dataBitLength));
        offset += splitPacketChannel->splitPacketList[j]->dataBitLength;
    }

to

    BitSize_t offset = 0;
    for (j=0; j < splitPacketChannel->splitPacketList.Size(); j++)
    {
//
// GARRY - RETRIEVE SPLIT PACKETS IN THE PROPER ORDER
//
        splitPacket = NULL;

        for ( int a = 0; a < splitPacketChannel->splitPacketList.Size(); a++ )
        {
            if ( splitPacketChannel->splitPacketList[a]->splitPacketIndex != j )
                continue;

            splitPacket = splitPacketChannel->splitPacketList[a];
            break;
        }

        if ( splitPacket == NULL ) 
        splitPacket = splitPacketChannel->splitPacketList[j];

        memcpy( internalPacket->data + BITS_TO_BYTES( offset ), splitPacket->data, (size_t)BITS_TO_BYTES( splitPacket->dataBitLength ) );
        offset += splitPacket->dataBitLength;
//
// END GARRY
//
    }

[Cleroth]

Having a serious issue like this and no replies from the developers doesn't exactly make RakNet look very good.

[vrripoll]

it, is the code of previous version, it think all split packet have the same size, it store the size into splitPacketPartLength=BITS_TO_BYTES(splitPacketChannel->firstPacket->dataBitLength);

and fill the memory with
memcpy(internalPacket->data+splitPacket->splitPacketIndex*splitPacketPartLength, splitPacket->data, (size_t) BITS_TO_BYTES(splitPacketChannel->splitPacketList[j]->dataBitLength));

and use splitPacketIndex to put the split pack in this position.

but i am not sure this code it is ok and all split packet have the same size.

----------- CODE -----------------------------
'''
InternalPacket * ReliabilityLayer::BuildPacketFromSplitPacketList( SplitPacketChannel *splitPacketChannel, CCTimeType time )
{
#if PREALLOCATE_LARGE_MESSAGES==1
InternalPacket *returnedPacket=splitPacketChannel->returnedPacket;
RakNet::OP_DELETE(splitPacketChannel, FILE, LINE);
(void) time;
return returnedPacket;
#else
unsigned int j;
InternalPacket * internalPacket, *splitPacket;
int splitPacketPartLength;
internalPacket = CreateInternalPacketCopy( splitPacketChannel->splitPacketList[0], 0, 0, time );
internalPacket->dataBitLength=0;
for (j=0; j < splitPacketChannel->splitPacketList.Size(); j++)
internalPacket->dataBitLength+=splitPacketChannel->splitPacketList[j]->dataBitLength;
splitPacketPartLength=BITS_TO_BYTES(splitPacketChannel->firstPacket->dataBitLength);

internalPacket->data = (unsigned char*) rakMalloc_Ex( (size_t) BITS_TO_BYTES( internalPacket->dataBitLength ), _FILE_AND_LINE_ );
internalPacket->allocationScheme=InternalPacket::NORMAL;

for (j=0; j < splitPacketChannel->splitPacketList.Size(); j++)
{
    splitPacket=splitPacketChannel->splitPacketList[j];
    memcpy(internalPacket->data+splitPacket->splitPacketIndex*splitPacketPartLength, splitPacket->data, (size_t) BITS_TO_BYTES(splitPacketChannel->splitPacketList[j]->dataBitLength));
}

for (j=0; j < splitPacketChannel->splitPacketList.Size(); j++)
{
    FreeInternalPacketData(splitPacketChannel->splitPacketList[j], _FILE_AND_LINE_ );
    ReleaseToInternalPacketPool(splitPacketChannel->splitPacketList[j]);
}
RakNet::OP_DELETE(splitPacketChannel, __FILE__, __LINE__);

return internalPacket;

#endif
}
'''

[Cleroth]

I might want to wrape that code with ```

[vrripoll]

sorry i only speak a little english

[Cleroth]

Put ``` at the start and end of your code.

[Cleroth]

That's almost right. You have to include new lines before and after it. So like

'''
Code
Code
'''

[vrripoll]

sorry i try it,y put ''' but it is not ok

[Cleroth]

It's ```, I just put ''' as an example so it didn't show as code. You still have to put a new line before your last one.

[vrripoll]

in the code of GARRY i dont understan how this part of the code it is possible

if ( splitPacket == NULL )
splitPacket = splitPacketChannel->splitPacketList[j];

how ist si posible not found splitPacket by splitPacketIndex

[Cleroth]

Packet editing? Packet corruption? Dunno.

[vrripoll]

i think if it not found splitPacket , it is a error and this mesage must be discard.
if use splitPacket = splitPacketChannel->splitPacketList[j]; it must be copying data in a Incorrect position

[Mellnik]

Can this be fixed and officially be committed on the repo? Ever since Oculus bought it there are no big updates...

[Cleroth]

RakNet has been abandoned, as is shown on the website.

[biller23]

Maybe Oculus, the new owner of RakNet, is too busy with VR to care about this project for now...

[Mellnik]

Fine, Oculus shall then assign someone repo admin rights so he can process pull requests.

[Cleroth]

I'm not really sure why they bought something to leave it die. It certainly at least needs someone to process pull requests, so we can include fixes like Garry's.

[Brybry]

Maybe someone with the know-how will start a community fork

Thanks for the fix @garrynewman! I was beating my head against this bug for hours :(
As noted by larku, it seems e97c4bb introduced this bug.

[larku]

I've got a fork ( https://github.com/larku/RakNet ) that includes most of the pending pull requests from the official if that's of use to you. I'm also happy to accept pull requests into this fork.

[Cleroth]

Seems the problem described in this ticket is still present in your fork.

[larku]

Hey @Cleroth, that fork includes:

larku@c15dd90

Which should address this.

[Cleroth]

Oh... so it was fixed long ago in a different way...
Thanks. I might actually give RakNet a try now.