search

facebookarchive / RakNet

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

BitStream ctor does not memset the data array to all 0, which could cause incorrect write of another bitstream #76

Open Kiddinglife opened 8 years ago

Kiddinglife commented 8 years ago

At lines between 220 and 227 in void BitStream::Write(BitStream *bitStream, BitSize_t bits2Write): } else { // Existing byte if( bitStream->data[bitStream->mReadPosBits >> 3] & ( 0x80 >> ( bitStream->mReadPosBits & 7 ) ) ) data[mWritePosBits >> 3] |= 0x80 >> ( numberOfBitsMod8 ); // Set the bit to 1else 0, do nothing }

Kiddinglife commented 8 years ago

this function supposes that the memory adress after write position is default initilized with value of 0. However, it may not be so

larku commented 8 years ago

Does this actually cause an issue? My understanding is that there can always be some 'junk' bits (byte padding) at the end of a BitStream once sent since it must send full bytes and does not send the number of bits used. The receiving bit stream does not know where the meat of the payload ends (in terms of bits). As such you need to always consider that and never read past what you know has been written (am I making any sense?)..

I could be totally wrong here, but my analysis suggests this.

OvermindDL1 commented 8 years ago

Unsure if changed since I last programmed on RakNet a decade ago, but the not setting it to 0 should not affect anything and was purposefully not done to help performance.

Kiddinglife commented 8 years ago
  1. It is very DANGEROUS to reply on compiler's default initialization for setting up all zeros.
  2. this method is very slow as it copies bit one after another. A easy and faster impl is reusing writebits() method to have a chance of memcpy().