[C++] Buffer Handling Code

[Mephisto]

May I see some example buffer handling code used in other people's bots?  Basically the code which handles modifying the buffer to make room, setting where to start receiving data into the buffer, iterating through each packet in the buffer, handling split messages, etc.

Here's mine which seems to not work as it should.  The problem is that it's randomly dropping and I suspect it has to do with the buffer code (though it's quite possible it has to do with something else).  If anyone sees anything wrong with it could you please inform me...or give me suggestions for improvement.  Thanks

This is the completion routine for receiving data.  It is called by the actual completion routine which is static, but this function does all of the work.  Variables bufstart and buflen are persistant and are private members of the connection class.  Function ParseData is called polymorphically from either BNLSConnection or BnetConnection derive from the generic class Connection.  ParseData handles the buffer information by doing various things, but class Connection does all of the buffer modifications and handling.

Code Select Expand

VOID Connection::RecvComplete(DWORD dwError, DWORD dwTransferred, LPWSAOVERLAPPED lpOverlapped, DWORD flags)
{
if (dwTransferred == 0) // disconnected
return;
global->bnetConnect->recvPackets++;
buflen = dwTransferred;
ParseData(RecvBuffer);
if (global->disconnected)
return;
if (((signed)sizeof(RecvBuffer) - (signed)(bufstart + buflen)) < 512) { // if there's not enough room left in the buffer
memmove(RecvBuffer, &RecvBuffer[bufstart], buflen);
bufstart = 0;
}
WSARecvBuf.buf = &RecvBuffer[bufstart + buflen];
WSARecvBuf.len = sizeof(RecvBuffer) - (bufstart + buflen);
if (WSARecv(_socket, &WSARecvBuf, 1, &bytes, &flags, &RecvOv, StaticRecvComplete) == SOCKET_ERROR && GetLastError() != WSA_IO_PENDING)
cout << Timestamp() << "Error: Could not receive data from server " << _server << ": " << WSAGetLastError() << endl;
}

Here is the ParseData function which is specific in its implementation for the server its parsing data from.  But nontheless it takes the buffer which is handled from the generic class Connection and parses it accordingly, and then modifying the persistant buflen and bufstart variables.  buflen is the dynamic length of the buffer decreasing as each packet is parsed, and bufstart is the buffer position where data from that packet received should be looked for and where data should be inserted into the buffer when received.

Code Select Expand

VOID BnetConnection::ParseData(LPSTR RecvBuffer)
{
WORD packetlen = 0;
while ((DWORD)buflen >= 4 && buflen >= *(WORD *)&RecvBuffer[bufstart + 2]) {
if ((BYTE)RecvBuffer[bufstart] != 0xFF) {
cout << Timestamp() << "Error: Received an invalid packet class!: " << hex << (BYTE)RecvBuffer[bufstart] << endl;
Disconnect();
/*ZeroMemory(RecvBuffer, sizeof(RecvBuffer));
bufstart = 0;
buflen = 0;
packetlen = 0;
return;*/
}
packetlen = *(WORD *)&RecvBuffer[bufstart + 2];
switch (RecvBuffer[bufstart + 1]) {
case SID_NULL:
Recv_NULL();
break;
case SID_PING:
Recv_PING();
break;
case SID_AUTH_INFO:
Recv_AUTH_INFO();
break;
case SID_AUTH_CHECK:
Recv_AUTH_CHECK();
break;
case SID_LOGONRESPONSE2:
Recv_LOGONRESPONSE2();
break;
case SID_AUTH_ACCOUNTLOGON:
Recv_AUTHACCOUNTLOGON();
break;
case SID_AUTH_ACCOUNTLOGONPROOF:
Recv_AUTHACCOUNTLOGONPROOF();
break;
case SID_ENTERCHAT:
Recv_ENTERCHAT();
break;
case SID_CHATEVENT:
Recv_CHATEVENT();
break;
default:
cout << Timestamp() << "Error: Unsupported message received!" << endl;
break;
}
sban = FALSE;
bufstart += packetlen;
buflen -= packetlen;
}
}

The if conditional where it checks to see if the element bufstart evaluates to is (BYTE)0xFF tends to fail a lot (making the conditional pass) causing a Disconnection, but I don't know why.

[MyndFyre]

This is brand-new code -- I just wrote it Thursday night.  You can see the entire class here. 

[edit: I just saw you'd prepended the topic with [C++].  This is C#, but you did ask for "code used in other people's bots".  So ]

Let me tell you how the class works so that you understand (more or less) what's going on:

This is the base class that actually provides connection functionality (there is a base class that this one derives from, but it makes no network connections).  This code uses the .NET Socket/NetworkStream asynchronous communication methods -- the function I'll post below is the callback that is passed into NetworkStream instance.BeginRead(byte[], int, int, AsyncCallback, object) method.  The asynchronous callbacks are private, and then the async callbacks call OnXxx methods, which are protected and virtual.

Because I want to use multithreading, I want to ensure that I'm parsing packets in the same order that I'm receiving them.  So, I use a Mutex object to synchronize access to the given object.

There is a general disclaimer: this code is missing a condition to check to make sure that the data buffer read from the network stream is long enough to hold the entire packet the BNCS header describes (i.e. the buffer in the code is shorter than the word in the header says it should be).  But aside from that, this receive handler *should* work (I expect to have that validated and connecting by Monday).  The library is designed to be able to support language-dependent strings, which is what the Language.Get function provides.  T is an alias for System.Diagnostics.Trace, defined at the top via "using T = System.Diagnostics.Trace;".

Code Select Expand


private void Bncs_DataReceived(IAsyncResult iar)
{
#if DEBUG
T.WriteLine("Entering OpenGameProtocol::Bncs_DataReceived(IAsyncResult)", "Method Call");
#endif
// len stores the length of data actually read from the stream.
int len = 0;
try
{
len = this.m_nsBncs.EndRead(iar);
if (len == 0)
{
#if DEBUG
T.WriteLine("Zero-length buffer read from socket in OpenGameProtocol::Bncs_DataReceived(IAsyncResult).", "Read Error");
#endif
// if the received data length is 0, there is a chance we disconnected.  Check.
if (!this.m_sckBncs.Connected)
throw new SocketException(System.Runtime.InteropServices.Marshal.GetLastWin32Error());
}
}
catch (SocketException se)
{
// EventHost provides event notification to any other objects that may require it.
this.EventHost.OnConnectionError(
#if MULTILANGUAGE
Language.Get("error-receiving-bncs-disconnected"),
#else
"There was an error receiving data from the Battle.net Chat Server: you have been disconnected and possibly IP-banned by the remote host.",
#endif
true, se);
// disconnect on a receive error.
Disconnect();
return;
}

// create a new buffer to store the data for later processing.
byte[] inBuf = new byte[len];
Array.Copy(this.m_bufBncs, 0, inBuf, 0, len);
// create a new buffer to read.
this.m_bufBncs = new byte[TCP_BUFFER_SIZE];

if (this.m_sckBncs.Connected)
{
// resume the read operation.
this.m_nsBncs.BeginRead(m_bufBncs, 0, TCP_BUFFER_SIZE, this.BncsDataReceived, this.m_sckBncs);
}

/****** CRITICAL SECTION ******/
// ensure that only one thread has access to this object right now.
m_mutBncsParse.WaitOne();

/*
* The code here is used to ensure that only one buffer at a time is set
* to receive BNCS incoming packets.  The reason we use mutexes here is so that
* this section of code is only executed by one thread at a time, which allows
* us to make sure that BNCS packets are processed in the order they were sent.
* The same process is used by the BNLS incoming data handler.
*/

if (len == 0)
{
// Exit the critical section so we don't have a major hang.
m_mutBncsParse.ReleaseMutex();
return;
}
else
{
// in reality the loop should be out here.  Oh well.
// it will be fixed soon enough.
short packetsPerceivedLength = BitConverter.ToInt16(inBuf, 2);
// If the packet thinks it is as long as it really is, there is only one packet contained.
if (packetsPerceivedLength == len)
{
OnBncsDataReceived(inBuf);
}
else
{
byte[] remainingBuffer = new byte[len];
Array.Copy(inBuf, 0, remainingBuffer, 0, len);
int idx = 0;
// the length of each connected packet is added to idx at each iteration.
// therefore the loop will terminate after all connected packets are processed.
while (idx <= len)
{
// Copy the next full BNCS packet into its own buffer.
// The first iteration will be the first BNCS packet.
byte[] firstBuffer = new byte[packetsPerceivedLength];
Array.Copy(remainingBuffer, idx, firstBuffer, 0, packetsPerceivedLength);
// Create the delegate that will be used to process the packet.
// Note that this is an OnXxx method.
AsyncPacketCallback apc = new AsyncPacketCallback(this.OnBncsDataReceived);
// Advance the current index by the size of the packet
idx += packetsPerceivedLength;
// Get the next packet's perceived length.
packetsPerceivedLength = BitConverter.ToInt16(remainingBuffer, idx + 2);
// Invoke the method.
apc.BeginInvoke(firstBuffer, null, null);
}
}
}

// we're done with this section of code.  Let another thread at it!
m_mutBnlsParse.ReleaseMutex();
/****** END CRITICAL SECTION ******/
}


I think the code is commented rather well, so if you have any other questions, shoot me a PM.

[Mephisto]

As always thank you for the response.  Btw, whenever BNCS sends you a 0-length buffer you've been disconnected (someone correct me if I am wrong though).  I notice that in your code you check first to see if you're disconnected, which I suppose is more or less better than assuming that it's been disconnected (which if I am write, is).  Btw, are you adding support for split messages?  I didn't look at it too thoroughly, but it looks like all you're doing is checking if you have just one packet, or multiple packets in one TCP stream.

[MyndFyre]

As always thank you for the response.  Btw, whenever BNCS sends you a 0-length buffer you've been disconnected (someone correct me if I am wrong though).  I notice that in your code you check first to see if you're disconnected, which I suppose is more or less better than assuming that it's been disconnected (which if I am write, is). 

I've noticed that .NET is funny about this particular aspect: sometimes during an async read operation, when no data has come in, and you begin an async write operation, the async read operation will return with 0 bytes in the buffer.  I check to make sure that the socket hasn't disconnected so that, if that's the reason that the operation returned, I can resume waiting to read.

Btw, are you adding support for split messages?  I didn't look at it too thoroughly, but it looks like all you're doing is checking if you have just one packet, or multiple packets in one TCP stream.

There is a general disclaimer: this code is missing a condition to check to make sure that the data buffer read from the network stream is long enough to hold the entire packet the BNCS header describes (i.e. the buffer in the code is shorter than the word in the header says it should be).

[Mephisto]

I thought you were describing in your disclaimer that there is no code to check whether there is enough space left in your buffer to handle another receive.  Mis-read. 

[Mephisto]

So no one sees anything wrong with my buffer code?  Or has example buffer code to share so I can improve mine? 


Edit: Just had a thought; perhaps it's one of my compiler switches that's possibly causing it to always 'drop'.  IIRC when people complained my by dropped the application itself terminated.  Could this be because of the /GS (Buffer Security Check) enabled in the release configuration causing it to 'drop' (or close) when it detects that there was a buffer overrun (according to this article: http://www.developer.com/security/article.php/11580_3417861_2) the compiler keeps a cookie, and if it's modified (the result of a buffer overrun) it causes the application to close if the security exception it throws is not handled...

[Eibro]

From first glance, you should have buflen += dwTransferred. What if there is already a partial packet in the buffer when you recv?

Also, why not actually do some work in ParseData and parse the data for each individual packet and call a (pure) virtual function with the parsed data. Then you can inherit from a base implementation and catch the different events you want to process.