WinSock, identifying a packet

Started by
46 comments, last by hplus0603 13 years ago
Yes, it could. It could look like mine =P

More seriously, use memcpy() or std::copy() for non-character data. Also there was a reason I separated the ChatHeader into a separate struct from the character data.
Advertisement
warning C4200: nonstandard extension used : zero-sized array in struct/union Cannot generate copy-ctor or copy-assignment operator when UDT contains a zero-sized array

warning C4996: 'std::copy': Function call with parameters that may be unsafe - this call relies on the caller to check that the passed values are correct.

something to be worried about?
You can change the zero sized array to 1 character, or to some arbitrary limit you're happy to set. You'd need to double check to ensure you are using the right sizes in all locations (sizeof() for allocation, the logical size for send/recv).

I used std::copy in a safe way. That warning there is a non standard attempt by Microsoft to deprecate some of the standard APIs. Whether that concerns you is up to you, but it doesn't bother me.
when you copy from a string to Char buffer, will you corrupt the data? Or does a nul terminated char buffer look at same as a string in memory?


struct ChatHeader
{
unsigned int ID, recipient, tag;
};

struct ChatPayload
{
ChatHeader header;
char text[4];
};

void SendChatPacket(unsigned int recipient, unsigned int tag, const std::string &text)
{
ChatPayload *chat = reinterpret_cast<ChatPayload *>(std::malloc(sizeof(ChatHeader) + text.length()));
if(!chat)
{
// Error handling...
}
chat->header.ID = 1;
chat->header.recipient = recipient;
chat->header.tag = tag;
// chat->header.TextLen = text.length() if you wish
std::copy(text.begin(), text.end(), chat->text);

const char *data = reinterpret_cast<const char *>(chat);
// Send code here

std::free(chat);
}
I'm copying using the string's iterators, so there is no problem provided there is enough room. The representation of std::string is implementation defined, but to meet the standard specification there is generally a buffer of contiguous characters somewhere. They aren't necessarily NUL terminated though, the standard allows them not to be. The implementation can be lazy about NUL terminating them, in case you rarely call c_str().

Instead of std::copy() you could use strcpy(chat->text, text.c_str()) if you are more comfortable with such functions, or some buffer safe variant of strcpy() if you wish.

when you copy from a string to Char buffer, will you corrupt the data? Or does a nul terminated char buffer look at same as a string in memory?


You should not assume that the data in your chat packet is actually zero terminated, because an attacker may easily create a packet that says it's a chat packet, but doesn't zero-terminate the buffer.
Thus, all variable-length fields in a network protocol should generally be length-prefixed rather than use a terminator.
Additionally, you should verify that the length doesn't overflow the actual data you have in the packet -- again, because otherwise, an attacker could read your stack/heap by crafting a malformed packet.
enum Bool { True, False, FileNotFound };
I'm quite pleased with this code


struct ChatHeader
{
unsigned int ID, recipient, tag;
};

void SendChatPacket(unsigned int recipient, unsigned int tag, const std::string &text)
{
char * buffer = (char*)malloc(sizeof(ChatHeader) + text.length());

ChatHeader* chat = reinterpret_cast<ChatHeader*>(buffer);
chat->ID = 1;
chat->recipient = recipient;
chat->tag = tag;

memcpy(buffer+sizeof(ChatHeader),text.c_str(), text.length());

//sendto(Socket, Buffer, sizeof(ChatHeader)+text.length(), 0, (sockaddr*)&RemoteAddress, sizeof(sockaddr));

free(buffer);
}

I'm quite pleased with this code



That contains neither a length of the string nor a zero termination. While you're sending the packet as a single UDP packet, in a real system, you'd put many messages into a single UDP packet, and wrap it in framing (to allow for re-sending dropped messages, etc), and at that point you can't know the length of the text without actually encoding it somehow.
enum Bool { True, False, FileNotFound };

This topic is closed to new replies.

Advertisement