WinSock, identifying a packet
It is too late for checks after the fact if you have a buffer overflow or other vulnerability in your packet parsing.
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?
Buffer[12]-Buffer[1399] will then contain the text, limited by the return value.
Any flaws in that?
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.
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.
All right. Please look over this code and tell me what you think:
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*);
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...
}
All right. Please look over this code and tell me what you think:
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 this thread. 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 you do not need any of those things in your application; it's like playing Russian roulette with network data!
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.
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.
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.
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);
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...
}
if(size>3){
ID = *(UINT*)Buffer;
switch(ID){
case 0:
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.
if(size>3){
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.
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.
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.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement