replace virgin memset with chad zero initializer
CPunch opened this issue · 6 comments
we use memset() to zero initialize most structs/arrays. this works fine, however the = {0} syntax looks much cleaner even though the compiler just inserts a memset anyways. this should be done after the refactor is merged
Are you willing to take this on now that refactor is merged?
I was ok with this change, but I remembered a day or two ago why we were initializing POD structs this way.
The difference between memset() and plain zero-initialization is that memset() overwrites the padding bytes at the end of the struct while zero-initialization doesn't - it only zeroes the memory occupied by the actual member fields. This is undesirable because we serialize many of those POD structs (predominantly packet structs and their member structs) directly into the packet protocol. Leaving the padding bytes uninitialized would lead to a leak of uninitialized memory to clients, which is a theoretical security issue.
So tl;dr, we should probably leave the memset() invocations alone.
I'm confused; there's padding at the end of the structs? I assumed we were sending the exact number of bytes as set in the defines file. If we're not doing that in all cases, we should be, right?
We send the exact number of bytes that the struct occupies, yes. If there's any padding bytes at the end of the struct, those are included.
You have a point that the protocol-related structs are all packed, which should mean there's no padding at the end of them. But they're not all 1-byte packed, which means some of them could still have uninitialized memory in between members of different sizes, where the first member is a smaller variable than the second (meaning the second has stricter alignment requirements, resulting in padding).
I vaguely recall that the exact padding granularity of each packet struct is defined such that there's never any gaps in practice, but I'm not at all sure of that. We must definitely verify that before making this type of change, and even then I don't know how I feel about cutting it that close; relying on only incidentally safe behavior.
I'm pretty confident that each packet struct is packed to have no padding in it, but I agree that the risk isn't worth it. Let's put this aside for now.
closing this for now