Two possibly related by errors.

[GoogleCodeExporter]

I run a modified copy of lidgren due to running on a Mono/.NET 2.0.  I 
discovered two (perhaps) related issues which I've subsequently fixed.  Both 
are due to miscounting bytes/bits, but I'm not exactly sure what sort of 
problems they might cause in your current version.  The issues still appear to 
be present, but I haven't tested it on your latest release.

My problem was running encryption and sending a large message consisting of 
only a string.  The string's length was roughly 9000 characters in length.  An 
assert consistently failed regarding the length in bytes that the 
ReadVariableUint32 returned compared to what was actually left in the packet.

Sorry how sloppy this error report is, but I'm on a tight deadline.  I thought 
sending in SOME error report would be better than no error report. :)
If you'd like a better explanation, I can do that at a later time, just let me 
know!

The first is related to the encryption code.  I've since modified mine to 
encode the payloadBitLength as a UInt32 since it makes sense to me to use 
something more than 16 bits.

In NetBlockEncryptionBase.cs:
--msg.EnsureBufferSize(dstSize * 8 + 2); // add 2 bytes for payload length at 
end
++msg.EnsureBufferSize((dstSize * 8) + (4 * 8)); // add 4 bytes for payload 
length at end

--msg.Write((ushort)payloadBitLength);
++msg.Write((UInt32)payloadBitLength);

--int numEncryptedBytes = msg.LengthBytes - 2; // last 2 bytes is true bit 
length
++int numEncryptedBytes = msg.LengthBytes - 4; // last 4 bytes is the true byte 
length.

--// read 16 bits of true payload length
++// read 32 bits of true payload length

--uint realSize = NetBitWriter.ReadUInt32(msg.m_data, 16, (numEncryptedBytes * 
8));
++UInt32 realSize = NetBitWriter.ReadUInt32(msg.m_data, 32, (numEncryptedBytes 
* 8));

So thats perhaps one part of the problem--not technically sure.
The other issue is that the developer appears to have inconsistently assumed 
that the WriteVariableUInt32 writes only 1 byte when, in fact, the 
WriteVariableUInt32 could write (or read) more than one byte if needed.  So, in 
the fix I implemented, I've replaced all WriteVariableUInt32 calls with 
Write(Uint32) calls since the cost of pre-calculating the length of the 
variable UInt32 far outways the savings earned for a relatively huge latency 
operation such as a network packet transmission.

In NetIncomingMessage.Write.cs:
In the internal void Write(string source) method:
--InternalEnsureBufferSize(m_bitLength + 8);
++InternalEnsureBufferSize(m_bitLength + (8 * 4));

In NetOutgoingMessage.Write.cs:
In the public void Write(string source) method:
-- EnsureBufferSize(m_bitLength + 8);
++ EnsureBufferSize(m_bitLength + (4 * 8);
-- WriteVariableUInt32(0);
++ Write((UInt32)0);

--EnsureBufferSize(m_bitLength + 8 + (bytes.Length * 8));
--WriteVariableUInt32((uint)bytes.Length);
++EnsureBufferSize(m_bitLength + (4 * 8) + (bytes.Length * 8));
++Write((UInt32)bytes.Length);

I'm sure I changed more than just that, but I'm sure you get the essence of the 
issue and hopefully a means of duplicating the problem.
Let me know if I can be of any help.  Of course, I'd be more than happy to send 
you my copy of the code if you think it would be useful.

Cheers!
Brett

Original issue reported on code.google.com by brettccl...@gmail.com on 28 Jun 2011 at 4:09

[GoogleCodeExporter]

Thanks for the feedback!

I've fixed the issue with only 16 bits of length for encrypted messages; now 32 
bits.

I'm keeping the 1 byte assumption in Write(string) tho. This has the following 
reasons:
1. Most string writes are below 127 characters; so any more exact calculation 
is most often wasted cpu
2. In most cases the message will be large enough anyway, since it should be 
recycled (and therefore preallocated) and also EnsureBufferSize() overallocates 
4 bytes when enlarging
3. A missed prediction in EnsureBufferSize() is no big deal - it will just 
reallocate (and overallocate, reducing the chance the next write will get the 
same problem)

--michael
EnsureBufferSize() actually overallocated 

Original comment by lidg...@gmail.com on 28 Jun 2011 at 6:53

• Changed state: Fixed
• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Uh... the last "EnsureBufferSize() actually overallocated" part is just a typo 
:-)

Original comment by lidg...@gmail.com on 28 Jun 2011 at 6:55

• Added labels: ****
• Removed labels: ****