Sign in to follow this  
Tispe

WinSock, identifying a packet

Recommended Posts

Hi, I have some trouble figuring out how to identify which Struct a payload belongs to.

The client can send a packet containing either a info or chat struct.
[code]
Struct INFO{UINT ID; float x, y, z;}
Struct Chat{UINT ID; UINT tag; string text;}
[/code]

How does the server analyse the first UINT to determine which struct the recived packet belongs to then copy the packet into that struct?

[code]
Struct Test{UINT ID; /*more stuff here?*/}

Test Recv;
recvfrom(Socket, (char*)&Recv, sizeof(Test), 0, (sockaddr*)&RemoteAddress, &SizeInt);

//process Recv....
//RecvChat = Recv....
//RecvInfo = Recv.....

Switch(Recv.ID){
case: 0
chat(RecvChat);
break;
case: 1
info(RecvInfo);
break;
}
[/code]

Share this post


Link to post
Share on other sites
Two options:

Size is based on ID. Read just the ID, then use that to look up the size. Then read the remaining bytes.

Next option: Transmit the size of packet. The first marker is the size of the packet, not the ID. Read the size first, then read the remaining bytes.



While basing it on the ID is a valid option, it is not easily extended. If you get a packet you don't already know your system will crash. If you need to expand the size of a type of packet you cannot mix old and new versions. It is generally easiest to prepend the size of all variable-sized blocks of data.

For example, all serialized packets could be of the form:

{
16-bit size (or 32-bit size if you can imagine having bigger packets)
32-bit packet type (since that's what you wanted to use)
32-bit packet sequence number (not necessary but extremely useful for debugging)
... packet-type specific data ...
}

Share this post


Link to post
Share on other sites
If I recvfrom() the packet is taken off the input queue. And I have no idea how long the payload is, data may be lost or I can go overkill and read with a Char array[4096] buffer?

To get a "taste" of the payload I got to use the MSG_PEEK flag first to find the size? Then read the first UNIT to get the ID?

So two calls to recvfrom() to first get the size(PEEK), then copy the payload (no flag)?

so far I got:

[code]
Struct InfoPayload{UINT ID; float x, y, z;}
Struct ChatPayload{UINT ID; UINT tag; string text;}
Struct Test{UINT ID;}

Test Recv;
int PayloadSize = recvfrom(Socket, (char*)&Recv, sizeof(Test), MSG_PEEK, (sockaddr*)&RemoteAddress, &SizeInt);

Char Buffer[PayloadSize];
recvfrom(Socket, &Buffer, PayloadSize, 0, (sockaddr*)&RemoteAddress, &SizeInt);

// then check ID with switch...?
Switch(Recv.ID){
case: 0
chat(Buffer);
break;
case: 1
info(Buffer);
break;
}
[/code]


Or should I go:

[code]
Struct InfoPayload{UINT ID; float x, y, z;}

Struct ChatPayload{UINT ID; UINT tag; string text;}

Struct Test{UINT ID;}



Test Recv;

recvfrom(Socket, (char*)&Recv, sizeof(Test), MSG_PEEK, (sockaddr*)&RemoteAddress, &SizeInt);

Switch(Recv.ID){
case: 0
ChatPayload RecvChat;
recvfrom(Socket, (char*)&RecvChat, sizeof(ChatPayload), 0, (sockaddr*)&RemoteAddress, &SizeInt);
chat(RecvChat);
break;
case: 1
InfoPayload RecvInfo;
recvfrom(Socket, (char*)&RecvInfo, sizeof(InfoPayload), 0, (sockaddr*)&RemoteAddress, &SizeInt);
info(RecvInfo);
break;
}
[/code]


Can you please provide some code?

Share this post


Link to post
Share on other sites
I have read multiple people advising against MSG_PEEK. It is an unneccessary call. Just provide a buffer that is big enough to hold the largest packet your application is going to produce, then read the next packet. You are going to need it anyway! When you receive it, look at the first byte or wherever your descriptor is and act accordingly. Fin!

Edit:
Okay I just read your last post. Well I guess this right here is the one exceptional case where raw type-casting is preferrable. I would just read the packet in a char buffer and cast it to the right struct.
Another way to produce/read packets is using bitstreams, scrapping the use of different structs alltogether. That way you would wrap your incoming lump of data in a bitstream class that provides functions like readInt(), readFloat(), unpackVector3(), readBit(), etc. Good thing about this is, you can easily add compression functionality to the bitstream class.

Edit²:
Look up WSAIoctl() and the flag FIONREAD. It will return the size of the first packet in the queue.

Share this post


Link to post
Share on other sites
Thanks for the tip.

How would you extract the first UINT from a [code]Char buffer[1400][/code]?

[code]
Struct Test{UINT ID;}
Test Rcv = (Test)buffer;
UINT ID = Rcv.ID;
[/code]

or

[code]
char *myChar = buffer[0]+buffer[1]+buffer[2]+buffer[3];
WORD ID = reinterpret_cast<WORD>(myChar);
[/code]

or

[code]
char *myChar = buffer[0]+buffer[1]+buffer[2]+buffer[3];
UINT ID = reinterpret_cast<UINT>(myChar);
[/code]

Or some other thing?

Once I get the ID, I would know for example buffer[0]-buffer[32] have valid information. Could I then cast the buffer with 1400 bytes into a 32 byte Struct? would the remainding bytes be ignored? Will this take more time then the MSG_PEEK apporch?

Share this post


Link to post
Share on other sites
Be warned that casting from a raw char buffer into a struct has [b]many[/b] pitfalls and is generally highly unadvisable. It makes your implementation very brittle and susceptible to certain classes of simple corrupted-data denial of service attacks, for instance. Worse, it can lead to pretty easy arbitrary-code-execution attacks, meaning that your software can be used to completely take over a computer that's running it.

The potential problems include, but are by no means limited to:

[list][*]Packing and padding differences between clients (structures may not share identical byte-for-byte layouts)[*]Endianness differences between clients (individual bytes/words/etc. may not share bit order across platforms)[*]Bit patterns for things like floating point types may differ across platforms[*]Only POD structures may be safely used; anything with a virtual member function, for instance, will cause corruption[*]You cannot use any pointer types in your packet structures, because what's a valid pointer on one client probably isn't on a different client[*]Ditto for anything like a C++SL string, container, or other nontrivial class; you have to manually serialize that sort of stuff[/list]

Look into [i]serialization[/i], particularly [i]network serialization[/i]. [url="http://www.parashift.com/c++-faq-lite/serialization.html"]This FAQ[/url] is a decent place to start.

Share this post


Link to post
Share on other sites
Extracting the first UINT is easy.

[code]
char buffer[1000];
UINT first = *(UINT*)buffer;
[/code]

However ApochPIQ is right. Pointer typecasting can be very dangerous if you're not absolutely sure what you are doing. Also, sending structs over the net is pretty easy to get you started but sooner or later you should switch to a different serialization mechanism.

Share this post


Link to post
Share on other sites
Thanks alot for the comments. I think I will stick to plain old data structs for now. I dont need pointers or that stuff. I will read 1400 bytes always so I dont have to MSG_PEEK. (which is faster and safe?)

[code]
Struct InfoPayload{UINT ID; float x, y, z;}
Struct ChatPayload{UINT ID, tag, TxtLen; char Text[1200];}

Char Buffer[1400];
UINT ID;

while(true){
recvfrom(Socket, Buffer, 1400, 0, (sockaddr*)&RemoteAddress, &SizeInt);
ID = *(UINT*)Buffer;

Switch(ID){
case: 0
info(*(InfoPayload*)Buffer, RemoteAddress);
break;
case: 1
chat(*(ChatPayload*)Buffer, RemoteAddress);
break;
default:
InvalidPacket(Buffer, RemoteAddress);
break;
}

}
[/code]

Share this post


Link to post
Share on other sites
You need to look at the return value of recvfrom(). It returns the size of the packet actually received, 0 if no packet was received, or negative values if an error occurred.

Remember, there is more than your game on the internet. If you happen to share a port with an existing service, you could easily receive arbitrary data. More seriously, you should be able to deal with a hacker constructing packets designed to crash your game, corrupt your game data (possibly to the advantage of the hacker) or even to remotely compromise the system running the server (arbitrary code execution).

You need to be super-paranoid when parsing packets.

Share this post


Link to post
Share on other sites
Yea, there will be a filtering layer in info() and chat() from which IPs may be banned for flooding or other detection methods. And after that another layer of handshaking/authentication for each IP. I just need a framework for sending and recieving different structs.

I though the return value only was valid for when MSG_PEEK was flagged, my mistake.

Share this post


Link to post
Share on other sites
Hmm. If I remove the text buffer from the chat struct so the size is fixed. Then Buffer[0] -Buffer[11] is the UINT ID, tag, TxtLen;
Buffer[12]-Buffer[1399] will then contain the text, limited by the return value.

Any flaws in that?

Share this post


Link to post
Share on other sites
Either way works, and you still need to be careful. For example, if you want to use the C string functions to manipulate the chat remember that they assume the presence of a NUL terminator. A hacker could construct a packet without a NUL terminator. You might need to use safer functions such as strncpy, or just forcibly insert a NUL (or even make your packet structure so that it doesn't require a NUL). If you want to convert the text into a std::string instance, don't use the convenient constructor, use one that allows you to limit the data it reads.

All the data from the network should be checked to ensure it is "sane" before it is used. In the example structures you most recently proposed, first ensure that at least a UINT sized packet arrived. Determine the exact packet type from that. Next, you would check that the packet is exactly as long as you expect or allow them (you might want to support variable sized packets later on, to conserve bandwidth).

For chat packets, you would also check that TexLen <= 1200, and then use the std::string constructor(const char *, int) to construct a string from the data. If you support any kind of markup in the chat content, you should also sanity check this too.

You could also do non-security related checks, such as seeing if the floating point values in the packets are "reasonable". This might help you find bugs in your packet building logic (e.g. you create a new packet type but accidentally re-use the "Info Payload Id" on the sending side, which results in garbage on the receiving side). This could be done in debug mode only.

Share this post


Link to post
Share on other sites
All right. Please look over this code and tell me what you think:

[code]
struct InfoPayload{UINT ID; float x, y, z;};
struct ChatPayload{UINT ID, tag, TxtLen;};

void chat(char*, int*, sockaddr_in*);
void info(char*, int*, sockaddr_in*);
void InvalidPacket(char*, int*, sockaddr_in*);
[/code]

[code]
DWORD WINAPI RecvThread(LPVOID lp)
{
char Buffer[1400];
UINT ID;
int size;
sockaddr_in RemoteAddress;

while(true){
size = recvfrom(Socket, Buffer, 1400, 0, (sockaddr*)&RemoteAddress, &SizeInt);
ID = *(UINT*)Buffer;
Buffer[size] = '\0';

if(size>3){
switch(ID){
case 0:
info(Buffer, &size, &RemoteAddress);
break;
case 1:
chat(Buffer, &size, &RemoteAddress);
break;
default:
InvalidPacket(Buffer, &size, &RemoteAddress);
break;
}
}
else{
InvalidPacket(Buffer, &size, &RemoteAddress);
}
}

return 0;
}

void chat(char* Buffer, int* size, sockaddr_in* Address)
{
ChatPayload Chat = *(ChatPayload*)Buffer;

if(Chat.TxtLen != (UINT)(size - (int*)sizeof(ChatPayload))){
return;
}

char *ptr = Buffer+sizeof(ChatPayload); //moving pointer past struct header

std::string Text(ptr, (int)Chat.TxtLen);

}

void info(char* Buffer, int* size, sockaddr_in* Address)
{
if(size != (int*)sizeof(InfoPayload)){
return;
}

InfoPayload Info = *(InfoPayload*)Buffer;
}

void InvalidPacket(char* Buffer, int* size, sockaddr_in* Address)
{
//errors, log, ban IP ect...
}
[/code]

Share this post


Link to post
Share on other sites
[quote name='Tispe' timestamp='1302536094' post='4797154']
All right. Please look over this code and tell me what you think:[/quote]

So now, someone sends a 1400 byte packet. You have a 1400 byte buffer that you set the 1401st byte to 0, corrupting whatever memory came after the buffer.

What about when a string is not the last member in your structure? The code does it no good either.

Since you are using C++, consider using the one of the patterns mentioned in [url="http://www.gamedev.net/topic/598169-packet-data-types/"]this thread[/url]. Either setup packet reader/writer classes and manually build.parse fields or make use of the visit pattern so you do not have to mess with this low level casting stuff that you will probably eventually have to change.

For the packet handler, consider using a map of UINT to function pointers/boost::functions/functors/etc... if you do not want one large switch statement. In doing so, you can keep the logic of the packet handlers separate and more easily maintainable than having to work with all of them at once.

Also, since you are using UDP, how are you going to take into account duplicated, lost, or packets corrupted on an application level? If you don't, I can't imagine players being happy when their chat messages never go through or they don't get info packets telling them the position of the epic loot they just dropped (just examples, but you should get the point.)

Consider making use of an existing UDP library that handles this stuff for you if you are trying to add networking to a project. Otherwise, you will need to look into add your own protocol on top of the UDP layer before your application logic is added to handle packet ordering and simple reliability as needed. It'd be really hard to imagine [i]you[/i] do not need any of those things in your application; it's like playing Russian roulette with network data!

Share this post


Link to post
Share on other sites
You should check for minimal packet size before dereferencing the ID. This isn't a huge problem (the buffer is easily bigger than any reasonable implementation of "int") but that is the logical place to handle this.

Setting buffer[size] to NUL isn't valid here, recvfrom() could return a packet exactly 1400 bytes long, return 1400 into "size" and you would write past the end of the array.

You're doing some rather strange and dangerous things with your integer pointers. Try making your "size" parameters a value rather than a pointer. You could pass the address by const reference rather than by pointer too. Finally, consider casting to the correct structure type in the calling code.

There is also data size, packing and endian issues to be aware of, but this will give you something to digest for the moment.

Share this post


Link to post
Share on other sites
Thanks for the input, I wish to keep it simple and "safe" for now and not dwell too much in to libraries. I will ofc add a sequencing UINT later for packets and ACK routines. CRC will be added last I think.

[code]
struct InfoPayload{UINT ID; float x, y, z;};
struct ChatPayload{UINT ID, tag, TxtLen;};

void chat(char*, int, sockaddr_in);
void info(char*, int, sockaddr_in);
void InvalidPacket(char*, int, sockaddr_in);
[/code]

[code]
DWORD WINAPI RecvThread(LPVOID lp)
{
char Buffer[1400];
UINT ID;
int size;
sockaddr_in RemoteAddress;

while(true){
size = recvfrom(Socket, Buffer, 1399, 0, (sockaddr*)&RemoteAddress, &SizeInt);
Buffer[size] = '\0';

if(size>3){
ID = *(UINT*)Buffer;
switch(ID){
case 0:
info(Buffer, size, RemoteAddress);
break;
case 1:
chat(Buffer, size, RemoteAddress);
break;
default:
InvalidPacket(Buffer, size, RemoteAddress);
break;
}
}
else{
InvalidPacket(Buffer, size, RemoteAddress);
}
}

return 0;
}

void chat(char* Buffer, int size, sockaddr_in Address)
{
ChatPayload Chat = *(ChatPayload*)Buffer;

if(Chat.TxtLen != (UINT)(size - (int)sizeof(ChatPayload))){
return;
}

char *ptr = Buffer+sizeof(ChatPayload);

std::string Text(ptr, (int)Chat.TxtLen);
//broadcast chat to group(s)
}

void info(char* Buffer, int size, sockaddr_in Address)
{
if(size != (int)sizeof(InfoPayload)){
return;
}

InfoPayload Info = *(InfoPayload*)Buffer;
//update entity
}

void InvalidPacket(char* Buffer, int size, sockaddr_in Address)
{
//errors, log, ban IP ect...
}
[/code]

Share this post


Link to post
Share on other sites
[quote name='Tispe' timestamp='1302542297' post='4797189']
[code]
if(size>3){
ID = *(UINT*)Buffer;
switch(ID){
case 0:
[/code]
[/quote]

Just because you have 4 bytes doesn't mean you have the entire packet.

Typically, your receive loop will look something like this:

1) receive into whatever buffer space is left
2) check whether there's enough for a header in the buffer
3) if not, return
4) decode the header (without destroying the buffer)
5) is there enough data in the buffer to cover the packet as described by the header?
6) if not, return
7) actually decode the entire packet, remove it from the buffer, preserving whatever data might be after the current packet.
8) goto 2)

Note that this means you're preserving buffered data between invocations of recv() for a given client.

Share this post


Link to post
Share on other sites
[code]if(size>3){[/code] to make sure there are no errors and to get the UINT ID to determine the type of payload. If I read the Q3 article right packets under 1500 bytes will be unfragmented. I think my code so far covers the basics. I will prob take rip-offs advice and cast the buffer into a struct before calling the functions. So that I dont pass 1400 bytes each time when only 96 bytes is the payload.

I am also interested to know if multi-threading this will cause conflics when updating entities. I am thinking of adding a "in use" bool for the update entity functions, and let a reader wait a microsecond if the function is "in use". Or some sort of message queue.

Share this post


Link to post
Share on other sites
[quote name='Tispe' timestamp='1302545248' post='4797211']
I am also interested to know if multi-threading this will cause conflics when updating entities. I am thinking of adding a "in use" bool for the update entity functions, and let a reader wait a microsecond if the function is "in use". Or some sort of message queue.
[/quote]
The first will not work, unless you have a platform specific check-and-set operation.

A message queue can work, but I suggest you get a well written, debugged, fully functioning version from the web. Attempting to write your own will likely cause problems.




Writing thread-safe code is not easy, it requires serious planning and also discipline to follow strict rules and locking priorities. Even with those in place there are frequent bugs.

There were some great articles a few years back in DDJ: The first showed how to build a lockless queue that is thread safe, and (the author claimed) the code addressed all the concurrency issues. The follow up article two months later pointed out flaws in the code, why they existed, and corrected them, and detailed some situations where even then the code would fail if misused.


The decision to use multiprocessing in your game should not be taken lightly: while it does let you solve many problems, it also adds significant complexity.

You opted to stick with POD structs for simplicity. The added complexity of a serialization class is tiny in comparison to the complexity added by multiprocessing. I suggest you also avoid multiprocessing for the same reasons.

Share this post


Link to post
Share on other sites
Question, why aren't std::vector thread-safe? would it be so hard to make it thread-safe?

Also if one thread is a mutex reader and another is a mutex writer, will a read occur instanly with no interference even while the writer is writing?

Share this post


Link to post
Share on other sites
Thread safety isn't free, it has overhead. In cases where you aren't using threads, it would mean the data structure performs sub-optimally. Even when you are using threads, a thread-safe data structure isn't always enough. Thread safety needs to be done at the logical transaction level, which doesn't always map to operations on a data structure.

Mutex stands for mutual exclusion. Only one thread can enter mutex protected code at a time. If your thread API distinguishes between grabbing a read lock and a write lock, then it would be possible for multiple readers to execute concurrently. It is not generally safe for a write and read to occur concurrently, as the data might be in an inconsistent state while the writer is executing.

I agree with frob, you don't need threading here. Just process as many packets as you can every frame. Multi-threading will likely cause you problems and subtle, time dependent bugs. You have enough on your plate with networking.

Share this post


Link to post
Share on other sites
[quote name='Tispe' timestamp='1302548431' post='4797218']
Understood. I am just worried my main loop will get stuck or add lag spikes. I guess reading max 100 packets per frame or something might suffice, though it may create congestion.
[/quote]

Lag means that you can't process all the packets you need to process within the time frame that is available for processing.
If you defer some packets, then that just means that the packets you deferred will be lagged for sure, because you're not processing them.
If you have "chunky" requests from clients, then you probably should still decode and enqueue them as soon as they come in. The actual work, which may include asynchronous requests like reading files r getting data from a database server, would run asynchronously, and respond back to the user (or the next processing step) using some kind of callback or event queue mechanism.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this