Bug Tracker – Bug 773

Verify packet integrity with CRC32

Last modified: 2012-01-29 06:12:25 UTC
Bug 773 - Verify packet integrity with CRC32
Summary: Verify packet integrity with CRC32
Status: NEW
Alias: None
Product: Odamex
Classification: Unclassified
Component: Server & Client (show other bugs)
Version: (old) 0.6-dev
Hardware: All All
: P1 enhancement
Assignee: Odamex Bug Reporter
URL:
Depends on:
Blocks:
 
Reported: 2012-01-12 06:03 UTC by Dr. Sean
Modified: 2012-01-29 06:12 UTC (History)
1 user (show)

See Also:

Attachments

Patch for r2714 that implements packet CRC32 checking (5.45 KB, patch)
2012-01-12 06:03 UTC, Dr. Sean
Details | Diff
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dr. Sean 2012-01-12 06:03:19 UTC
Created attachment 329 [details]
Patch for r2714 that implements packet CRC32 checking

UDP provides a 16-bit checksum for verifying the integrity of a packet.  While this is relatively sufficient (only 1 in 65536 corrupted packets would be accepted), the UDP checksum is optional, which poses a greater problem.  If a client's TCP/IP stack does not use the checksum or (more likely) some router between the client and the server does not use the checksum, corrupted packets are not being detected.

While I am not sure of the impact this has for Odamex, other non-Doom games implement their own CRC32 check for each packet sent and received, dropping corrupted packets.  I have implemented this for Odamex and plan to test it in the near future with a few players I've come across who experience higher than normal network related bugs.  Hopefully this will allow me to gauge whether adding CRC32 checks to packets would be worth the extra 4-byte overhead per packet.
Comment 1 Russell Rice 2012-01-29 06:12:25 UTC
if other multiplayer games use this and what you say about the field being optional, I think its a must then

another thing is VLAs are a gcc extension, line 524 of i_net.cpp uses one which is msvc incompatible, if you can could you update the patch to use either alloca() or malloc/free combo instead?