OpenFusionProject / OpenFusion

Open source server for the FusionFall client
MIT License
351 stars 64 forks source link

replace virgin memset with chad zero initializer #260

Closed CPunch closed 1 year ago

CPunch commented 1 year ago

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

yungcomputerchair commented 1 year ago

Are you willing to take this on now that refactor is merged?

dongresource commented 1 year ago

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.

yungcomputerchair commented 1 year ago

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?

dongresource commented 1 year ago

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.

yungcomputerchair commented 1 year ago

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.

CPunch commented 1 year ago

closing this for now