[C++] Looking for constructive criticism on my PacketBuffer class.

[Sorc.Polgara]

Hi, I wrote a PacketBuffer class in C++ months ago and have just recently gone back to look over it and try to fix it up because I've been teaching myself how to make Dynamic Link Libraries and plan on incorporating the PacketBuffer class in my first actual useful DLL.  However, before I go forward and start making the DLL that would use the PacketBuffer class, I want to make sure that the code is DECENT enough to be put in it.

I'm a newb programmer and just want to get better so I am looking for the vL community's input and CONSTRUCTIVE criticism so that I can improve it and make it better before I use it in my DLL.

There are two files:  PacketBuffer.h and PacketBuffer.cpp

PacketBuffer.h

Code Select Expand


// Michael Souza
// PacketBuffer.h
//

#ifndef _PACKETBUFFER_H_
#define _PACKETBUFFER_H_

#include <windows.h>


//==========================================================================
// class PacketBuffer

class PacketBuffer
{
// public variables and functions
public:
PacketBuffer(void); // default constructor
PacketBuffer(size_t); // constructor
~PacketBuffer(void); // destructor

void insertFILETIME(FILETIME insFILETIME); // insert a filetime structure
void insertBOOL(BOOL insBOOL); // insert a BOOL
void insertDWORDs(const void* data, int size); // insert DWORDs
void insertDWORD(DWORD insDWORD); // insert a DWORD
void insertWORD(WORD insWORD); // insert a WORD
void insertBYTE(BYTE insBYTE); // insert a byte/character
void insertNTString(LPSTR insNTString); // insert a null-terminating string
void insertBuffer(LPSTR insBuffer); // insert a buffer
void insertNonNTString(LPSTR insNonNTString); // insert a non null-terminating string

LPSTR rawDumpBuf(void); // dump the raw buffer
LPSTR dumpBuf(void); // dump the buffer
LPSTR insertBNLSHeader(BYTE pID); // BNLS packet
LPSTR insertBNCSHeader(BYTE pID); // BNCS packet
size_t getBufSize(void); // get the buffer size

// private variables and functions
private:
size_t bufSize; // buffer length/size
size_t size; // temporary size/length
size_t pos; // stores the position
LPSTR buffer; // buffer
LPSTR tempBuf; // temporary buffer

int resizeBuf(size_t resize); // resize the buffer
int chkBufSize(size_t insSize); // check the buffer size
};

//==========================================================================
#endif


PacketBuffer.cpp

Code Select Expand


// Michael Souza
// PacketBuffer.cpp
// 10/13/05

//==========================================================================
// Header files
#include <windows.h>
#include <malloc.h>
#include <stdio.h>
#include "PacketBuffer.h"

//==========================================================================
// Default Constructor
PacketBuffer::PacketBuffer(void)
{
tempBuf = NULL;
buffer = NULL;
pos = 0;

if((buffer = (char*)malloc(128 * sizeof(DWORD))) == NULL){
printf("Error:  Unable to initialize the buffer size.\n");
}else{
bufSize = _msize(buffer);
}
}

//==========================================================================
// Constructor #2
PacketBuffer::PacketBuffer(size_t bsize)
{
tempBuf = NULL;
buffer = NULL;
pos = 0;

if((buffer = (char*)malloc(bsize)) == NULL){
printf("Error:  Unable to initialize the buffer size.\n");
}else{
bufSize = _msize(buffer);
}
}

//==========================================================================
// Destructor
PacketBuffer::~PacketBuffer()
{
delete [] buffer;
}

//==========================================================================
// Insert a FILTIME structure into the packet buffer
void PacketBuffer::insertFILETIME(FILETIME insFILETIME)
{
if(chkBufSize(sizeof(FILETIME)) == false){
printf("Error:  Cannot insert FILETIME.\n");
}else{
(-1)[(FILETIME*)(buffer + (pos += sizeof(FILETIME)))] = insFILETIME;
}
}

//==========================================================================
// Insert a DWORD into the packet buffer
void PacketBuffer::insertDWORD(DWORD insDWORD)
{
if(chkBufSize(sizeof(DWORD)) == false){
printf("Error:  Cannot insert DWORD.\n");
}else{
(-1)[(DWORD*)(buffer + (pos += 4))] = insDWORD;
}
}

//==========================================================================
// Insert a BOOL into the packet buffer
void PacketBuffer::insertBOOL(BOOL insBOOL)
{
if(chkBufSize(sizeof(BOOL)) == false){
printf("Error:  Cannot insert BOOL.\n");
}else{
(-1)[(BOOL*)(buffer + (pos += 4))] = insBOOL;
}
}

//==========================================================================
// Insert x amount of DWORDs into the packet buffer
void PacketBuffer::insertDWORDs(const void *data, int size)
{
if(chkBufSize(sizeof(size)) == false){
printf("Error:  Cannot insert DWORDs.\n");
}else{
memcpy(buffer + pos, data, size);
pos += size;
}
}

//==========================================================================
// Insert a WORD into the packet buffer
void PacketBuffer::insertWORD(WORD insWORD)
{
if(chkBufSize(sizeof(WORD)) == false){
printf("Error:  Cannot insert WORD.\n");
}else{
(-1)[(WORD*)(buffer + (pos += 2))] = insWORD;
}
}

//==========================================================================
// Insert a BYTE into the packet buffer
void PacketBuffer::insertBYTE(BYTE insBYTE)
{
if(chkBufSize(sizeof(BYTE)) == false){
printf("Error:  Cannot insert BYTE.\n");
}else{
(-1)[(BYTE*)(buffer + (pos += 1))] = insBYTE;
}
}

//==========================================================================
// Insert a null-terminating string into the packet buffer
void PacketBuffer::insertNTString(LPSTR insNTString)
{
if(chkBufSize(strlen(insNTString)) == false){
printf("Error:  Cannot insert Null-Terminating String.\n");
}else{
memcpy(buffer + pos, insNTString, strlen(insNTString) + 1);
pos += strlen(insNTString) + 1;
}
}

//==========================================================================
// Insert a non null-terminating string into the packet buffer
void PacketBuffer::insertNonNTString(LPSTR insNonNTString)
{
if(chkBufSize(strlen(insNonNTString)) == false){
printf("Error:  Cannot insert Non Null-Terminating String.\n");
}else{
tempBuf = new char[strlen(insNonNTString)];

strcpy(tempBuf, insNonNTString);

memcpy(buffer + pos, _strrev(tempBuf), strlen(tempBuf));
pos += strlen(tempBuf);
}
}

//==========================================================================
// Insert a buffer in the packet buffer
void PacketBuffer::insertBuffer(LPSTR insBuffer)
{
if(chkBufSize(_msize(insBuffer)) == false){
printf("Error:  Cannot insert buffer.\n");
}else{
memcpy(buffer + pos, insBuffer, _msize(insBuffer));
pos += _msize(insBuffer);
}
}

//==========================================================================
// Resizes the packet buffer
int PacketBuffer::resizeBuf(size_t resize)
{
if((buffer = (char*)realloc(buffer, ((resize)))) == NULL){
printf("Error:  Unable to reallocate/resize buffer.\n");
return false;
}
bufSize = _msize(buffer);
return true;
}

//==========================================================================
// Puts the BNCS header onto the packet buffer and returns the buffer
LPSTR PacketBuffer::insertBNCSHeader(BYTE pID)
{
if(resizeBuf(pos) == false){
printf("Error:  Unable to reallocate/resize buffer.\n");
return 0;
}else{
bufSize = (sizeof(BYTE) + sizeof(BYTE) + sizeof(WORD) + _msize(buffer));
tempBuf = new char[bufSize];
PacketBuffer pbuf(bufSize);

pbuf.insertBYTE(0xFF);
pbuf.insertBYTE(pID);
pbuf.insertWORD(bufSize);
pbuf.insertBuffer(buffer);

memcpy(tempBuf, pbuf.dumpBuf(), bufSize);

return tempBuf;
}
}

//==========================================================================
// Puts the BNLS header onto the packet buffer and returns the buffer
LPSTR PacketBuffer::insertBNLSHeader(BYTE pID)
{
if(resizeBuf(pos) == false){
printf("Error:  Unable to reallocate/resize buffer.\n");
return 0;
}else{
bufSize = (sizeof(BYTE) + sizeof(WORD) + _msize(buffer));
tempBuf = new char[bufSize];
PacketBuffer pbuf(bufSize);

pbuf.insertWORD(bufSize);
pbuf.insertBYTE(pID);
pbuf.insertBuffer(buffer);

memcpy(tempBuf, pbuf.dumpBuf(), bufSize);

return tempBuf;
}
}

//==========================================================================
// Dumps the packet buffer
LPSTR PacketBuffer::dumpBuf(void)
{
if(resizeBuf(pos) == false){
printf("Error:  Unable to reallocate/resize buffer.\n");
}

tempBuf = new char[pos];
memcpy(tempBuf, buffer, _msize(buffer));

return tempBuf;
}

//==========================================================================
// Dumps the raw packet buffer
LPSTR PacketBuffer::rawDumpBuf(void)
{
return buffer;
}

//==========================================================================
// Gets/returns the packet buffer size
size_t PacketBuffer::getBufSize(void)
{
return _msize(buffer);
}

//==========================================================================
// Performs various size/length checks on the buffer
int PacketBuffer::chkBufSize(size_t insSize)
{
if(bufSize < (pos + insSize)){
printf("Error:  Not enough buffer space. Resizing...\n");
return false;
}
if(bufSize <= (pos)){
printf("Error:  Invalid buffer positon.\n");
return false;
}
if(bufSize == 0){
printf("Error:  Buffer size is 0.\n");
return false;
}
if(buffer == NULL){
printf("Error:  Buffer is empty.\n");
return false;
}

return true;
}

//==========================================================================


Let me show a simple of example of how I use it.  NOTE:  The code is completely made up off the top of my head, so these are not actual BNLS or BNCS packets, although the packet IDs are real IDs.

This example assembles a buffer with the BNCS format head.

Code Select Expand


PacketBuffer pBuffer;
LPSTR buf;

pBuffer.insertDWORD(0x14);
pBuffer.insertWORD(0x0C);
pBuffer.insertNTString("Hello World");
buf = pBuffer.insertBNCSHeader(0x0D);


This example assembles a buffer with the BNLS format head.

Code Select Expand


PacketBuffer pBuffer;
LPSTR buf;

pBuffer.insertDWORD(0x17);
pBuffer.insertWORD(0x0D);
pBuffer.insertNTString("Sorc.Polgara");
buf = pBuffer.insertBNLSHeader(0x0F);


This code doesn't put any packet header... just returns the packet buffer.

Code Select Expand


PacketBuffer pBuffer;
LPSTR buf;

pBuffer.insertDWORD(0x12);
pBuffer.insertWORD(0x0E);
pBuffer.insertNTString("Hello World");
buf = pBuffer.dumpBuf();


Any suggestions, comments, and/or tips on ways to maybe optimize the code, make it more efficient, etc.?


Thanks.

[Eibro]

Just from looking at your interface:
Don't have "make BNCS packet" and "make BNLS packet" member functions. This severly limits the codes reuse. Trade "insertFILETIME" for some sort of generic, templated "insertSTRUCTURE" function. Then you could do buf.insertStruct<FILETIME>( p ). Likewise, for my first suggestion, instead of writing buf.bnlsBuffer( 0 ) write createBNLSBuffer( buf )

[Sorc.Polgara]

Just from looking at your interface:
Don't have "make BNCS packet" and "make BNLS packet" member functions. This severly limits the codes reuse. Trade "insertFILETIME" for some sort of generic, templated "insertSTRUCTURE" function. Then you could do buf.insertStruct<FILETIME>( p ). Likewise, for my first suggestion, instead of writing buf.insertBNLSHeader( 0 ) write createBNLSBuffer( buf )

Hmmm if I were to do this createBNLSBuffer( buf ) instead of this buf.insertBNLSHeader( 0 ), I would need to pass it a buffer...  errr, shit, I have to think of a way to say what I'm trying to say...

Ok, so like for a example of code if I were to make the change would be something like this?

Code Select Expand


PacketBuffer pBuffer;
LPSTR tempBuf, buffer;

pBuffer.insertDWORD(0x0C);
pBuffer.insertWORD(0x15);
pBuffer.insertNTString("I'm newb");
tempBuf = pBuffer.dumpBuf();
buffer = pBuffer.createBNLSBuffer(tempBuf);


Just trying to figure out what you are saying to do...


You see, I have this member function named dumpBuf() that can be used instead of either bnlsBuffer() or insertBNLSHeader()... that way you're not forced to have to choose between making the packet a BNLS or BNCS format... You can simply make the packet with no packet head at all.  The insertBNLSHeader() and insertBNCSHeader() member functions are like optional I guess you could say.

I'm willing to try your suggestion, I just don't know exactly what you mean.


Also, you suggested making a more generic structure function instead of the insertFILETIME() function.  I vaguely remember reading about making Templates... it has been awhile since Junior year of HS.  I'll have to look it up.  Good thing I still have all my C++ books.

[rabbit]

Using printf() isn't a great idea.  You should throw an error instead, just in case the person using the DLL isn't writing a console based bot, or is using VB.

[MyndFyre]

The .insertStructure method could be templated as:

Code Select Expand


void PacketBuffer::insertStructure(LPVOID lpData, UINT uLength);


Also, you don't need an "insert non-NT string" method.  That terminology came around for languages that don't auto-include a null terminator.  C/++ allows for DWORD constants to be specified by chars:

Code Select Expand


unsigned int ix86 = 'IX86'; // shows in memory as 68XI


So, you could do it with just packet->insertDWORD -- plus you'd be able to have the text in the right direction.

[shout]

You might want to look into using realloc() instead of malloc(), it resizes an allocated block of memory. I'm not sure, but I think excessive use of realloc() and malloc() may lead to fragmentation, so you might want to have the buffer resize itself in increments to avoid this.

Also, you seem to be mixing ints and unsigned ints. While I don't see the problem with this if the buffer is kept from getting very large, it could make for some interesting debuging (if you forgot to initialize something and you try allocate 34623781 bytes...?).

[Kp]

C/++ allows for DWORD constants to be specified by chars:

Code Select Expand

unsigned int ix86 = 'IX86'; // shows in memory as 68XISo, you could do it with just packet->insertDWORD -- plus you'd be able to have the text in the right direction.

If I remember correctly, support for multi-byte literals is a Microsoft-extension to the standard.  GCC supports it too, but complains strongly because it's an implementation dependent construct (i.e. may not give you the same results on compilers that aren't GCC).  Microsoft's compiler silently accepts this implementation dependent construct.

you shouldn't use malloc and delete together.   'new' is the standard way of allocating heap memory.

Actually, malloc was the standard way of allocating memory for a long time before new came along.  He should use whichever allocation method is most appropriate for the data type - and always match up the allocation/release types.  Typically, I use malloc/realloc/free for any "simple" data type (built-ins + any that I know will never have constructors due to compatibility reasons), since that grants the opportunity to realloc the size up and down as I please without a guaranteed copy.  Use new/delete for single instances of non-POD types, and use new[] / delete[] for arrays of non-POD types.

You might want to look into using realloc() instead of malloc(), it resizes an allocated block of memory. I'm not sure, but I think excessive use of realloc() and malloc() may lead to fragmentation, so you might want to have the buffer resize itself in increments to avoid this.

Even if it doesn't cause fragmentation, it's generally wasteful to do lots of small reallocations instead of a few significant ones.  Unless the program is badly memory bound, it's probably worth it to do as Shout suggests and accept having some unused space from time-to-time.

Also, you seem to be mixing ints and unsigned ints. While I don't see the problem with this if the buffer is kept from getting very large, it could make for some interesting debuging (if you forgot to initialize something and you try allocate 34623781 bytes...?).

Compiling with -Wall -Wextra should catch these.  For added paranoia, run it through a memory debugger such as Valgrind's Memcheck.

[MyndFyre]

If I remember correctly, support for multi-byte literals is a Microsoft-extension to the standard.  GCC supports it too, but complains strongly because it's an implementation dependent construct (i.e. may not give you the same results on compilers that aren't GCC).  Microsoft's compiler silently accepts this implementation dependent construct.

If I recall correctly, you were the one who told me that my language (C#) was broken because it didn't accept multibyte literals like C++. 

[Kp]

If I remember correctly, support for multi-byte literals is a Microsoft-extension to the standard.  GCC supports it too, but complains strongly because it's an implementation dependent construct (i.e. may not give you the same results on compilers that aren't GCC).  Microsoft's compiler silently accepts this implementation dependent construct.

If I recall correctly, you were the one who told me that my language (C#) was broken because it didn't accept multibyte literals like C++. 

That doesn't sound like something I'd say.  Link?

[Sorc.Polgara]

Thanks for the responses!

Using printf() isn't a great idea.  You should throw an error instead, just in case the person using the DLL isn't writing a console based bot, or is using VB.

Is something like this better?  Is using cout not a good idea?

Code Select Expand


//==========================================================================
// Insert a DWORD into the packet buffer
void PacketBuffer::insertDWORD(DWORD insDWORD)
{
try{
if(chkBufSize(sizeof(DWORD)) == false){
throw "Cannot insert DWORD.";
}
}catch( char * str ){
cout << "Error: " << str << endl;
}

(-1)[(DWORD*)(buffer + (pos += 4))] = insDWORD;
}


The .insertStructure method could be templated as:

Code Select Expand


void PacketBuffer::insertStructure(LPVOID lpData, UINT uLength);


Would this work?

Code Select Expand


//==========================================================================
// Insert a structure into the packet buffer
void PacketBuffer::insertStructure(LPVOID lpData, UINT uLength)
{
try{
if(chkBufSize(uLength) == false){
throw "Cannot insert the structure.";
}
}catch( char * str ){
cout << "Error: " << str << endl;
}

(-1)[(LPVOID*)(buffer + (pos += uLength))] = lpData;
}


You might want to look into using realloc() instead of malloc(), it resizes an allocated block of memory. I'm not sure, but I think excessive use of realloc() and malloc() may lead to fragmentation, so you might want to have the buffer resize itself in increments to avoid this.

Ok, so what your saying is that I should resize the buffer using realloc() every time I add something to the buffer using just the needed space.  For example when adding a DWORD to the buffer, resize the buffer so that it reallocates it 4 more bytes?

------
I'm so fucking newb.  What I know about C++ is basically the stuff from the very limited and basic High School level C++ course and what I've learned on my own.  I'm really regretting having made a bot in VB because it has made me ignorant.  C++ is so much more powerful, and because of that it is way more complicated.  It requires you to work more carefully with memory and stuff.  Plus there are so many little things that can make your code better that I wasn't taught.  The little tidbits.

EDIT:  Wow, do some of you guys program for a living?

[MyndFyre]

If I remember correctly, support for multi-byte literals is a Microsoft-extension to the standard.  GCC supports it too, but complains strongly because it's an implementation dependent construct (i.e. may not give you the same results on compilers that aren't GCC).  Microsoft's compiler silently accepts this implementation dependent construct.

If I recall correctly, you were the one who told me that my language (C#) was broken because it didn't accept multibyte literals like C++. 

That doesn't sound like something I'd say.  Link?

In C#, it is illegal to do 'STAR', so we pass it as a string instance as "STAR", and the code reverses it and inserts it as four bytes in a row.

How unfortunate.  Multi-character literals are so nice.  It really should support them.

I guess you didn't say it was "broken," but more along the lines of implied that it was deficient. 

Sorc: You shouldn't realloc() every time.  I suggest having a "count" variable that tracks how many bytes have been inserted, then when you're going to exceed that number of bytes, realloc() the buffer to be larger.  When you're preparing to send the data, only copy out the bytes that were actually occupied.  Using an appropriate auto-increase proportion size, you'll have good efficiency of both memory and speed.  realloc()'ing every time would slow down your code.

[Eibro]

Just from looking at your interface:
Don't have "make BNCS packet" and "make BNLS packet" member functions. This severly limits the codes reuse. Trade "insertFILETIME" for some sort of generic, templated "insertSTRUCTURE" function. Then you could do buf.insertStruct<FILETIME>( p ). Likewise, for my first suggestion, instead of writing buf.insertBNLSHeader( 0 ) write createBNLSBuffer( buf )

Hmmm if I were to do this createBNLSBuffer( buf ) instead of this buf.insertBNLSHeader( 0 ), I would need to pass it a buffer...  errr, shit, I have to think of a way to say what I'm trying to say...

Ok, so like for a example of code if I were to make the change would be something like this?

Code Select Expand


PacketBuffer pBuffer;
LPSTR tempBuf, buffer;

pBuffer.insertDWORD(0x0C);
pBuffer.insertWORD(0x15);
pBuffer.insertNTString("I'm newb");
tempBuf = pBuffer.dumpBuf();
buffer = pBuffer.createBNLSBuffer(tempBuf);


Just trying to figure out what you are saying to do...


You see, I have this member function named dumpBuf() that can be used instead of either bnlsBuffer() or insertBNLSHeader()... that way you're not forced to have to choose between making the packet a BNLS or BNCS format... You can simply make the packet with no packet head at all.  The insertBNLSHeader() and insertBNCSHeader() member functions are like optional I guess you could say.

I'm willing to try your suggestion, I just don't know exactly what you mean.


Also, you suggested making a more generic structure function instead of the insertFILETIME() function.  I vaguely remember reading about making Templates... it has been awhile since Junior year of HS.  I'll have to look it up.  Good thing I still have all my C++ books.

If they are "optional" they really shouldn't be part of the class interface then. Strive for an interface that is minimal and complete.

Of course you're going to need an additional buffer object if you move to the interface changes I suggested. You would write:

buf = createBnlsPacket( packetBuffer, 0 );

Where buf was usually something like auto_ptr< WSABufEx >, and WSABufEx was basically:

Code Select Expand

struct WSABufEx : public WSABUF {
    virtual ~WSABufEx() { delete[] data; }
    u_long pos;
};

[Eibro]

Thanks for the responses!

Using printf() isn't a great idea.  You should throw an error instead, just in case the person using the DLL isn't writing a console based bot, or is using VB.

Is something like this better?  Is using cout not a good idea?

Code Select Expand


//==========================================================================
// Insert a DWORD into the packet buffer
void PacketBuffer::insertDWORD(DWORD insDWORD)
{
try{
if(chkBufSize(sizeof(DWORD)) == false){
throw "Cannot insert DWORD.";
}
}catch( char * str ){
cout << "Error: " << str << endl;
}

(-1)[(DWORD*)(buffer + (pos += 4))] = insDWORD;
}


The .insertStructure method could be templated as:

Code Select Expand


void PacketBuffer::insertStructure(LPVOID lpData, UINT uLength);


Would this work?

Code Select Expand


//==========================================================================
// Insert a structure into the packet buffer
void PacketBuffer::insertStructure(LPVOID lpData, UINT uLength)
{
try{
if(chkBufSize(uLength) == false){
throw "Cannot insert the structure.";
}
}catch( char * str ){
cout << "Error: " << str << endl;
}

(-1)[(LPVOID*)(buffer + (pos += uLength))] = lpData;
}


You might want to look into using realloc() instead of malloc(), it resizes an allocated block of memory. I'm not sure, but I think excessive use of realloc() and malloc() may lead to fragmentation, so you might want to have the buffer resize itself in increments to avoid this.

Ok, so what your saying is that I should resize the buffer using realloc() every time I add something to the buffer using just the needed space.  For example when adding a DWORD to the buffer, resize the buffer so that it reallocates it 4 more bytes?

------
I'm so fucking newb.  What I know about C++ is basically the stuff from the very limited and basic High School level C++ course and what I've learned on my own.  I'm really regretting having made a bot in VB because it has made me ignorant.  C++ is so much more powerful, and because of that it is way more complicated.  It requires you to work more carefully with memory and stuff.  Plus there are so many little things that can make your code better that I wasn't taught.  The little tidbits.

EDIT:  Wow, do some of you guys program for a living?

If you need room for one more "DWORD", don't just allocate current size + sizeof DWORD, allocate twice the current size of the buffer. 1.5 times the current size of the buffer is good too. 1.5 is the strategy std::vector uses for growth. Also, why throw an exception? Either resize the buffer transparently, or return some error code.

[shout]

Code Select Expand


void buffer::resizebuffer(DWORD needed)
{
//Lets say you increment in 64 byte blocks for a battle.net
//buffer. This should be sufficent as most packets are not that long.
DWORD inc = 64;
DWORD mem_len = ((needed / inc) + (needed % inc > 0 ? 1 : 0)) * inc;
//This might look a little confusing. What this does is sets the length needed
//by the current data in the increment of 64
if (total_length - data_length > needed)
return;
//Does not needed to be resized.
mem_len += this->total_length;
this->data = realloc(this->data, mem_len);
}
Then, when you have any insert function...

Code Select Expand


resizebuffer(sizeof(n));


You might want to add extra checks to avoid allocating memory when it is not needed.