TheAssemblyArmada / Chronoshift

An open source re-implementation of Red Alert written in C++.
GNU General Public License v2.0
174 stars 24 forks source link

Code_Pointers() is using undefined behavior to do type punning #268

Closed mvdhout1992 closed 4 years ago

mvdhout1992 commented 4 years ago

Code_Pointers() is using reinterpret_cast to cast from pointer to integer. That's undefined behavior in C/C++. You need to use memcpy(). They didn't have time to fix this for the C++ 2020 standard but are working on it. See this recent talk by a C++ committee member where he talks about this issue:

https://www.youtube.com/watch?v=_qzMpk-22cc

Note also that this code only works because sizeof(int) is the same as the pointer size on x86. Machines aren't required to have the same pointer as native int size by C++. I'm not sure if a C++ compiler on x86 is required to do this.

Another thing is that memcpy() isn't allowed if the target and destination type sizes aren't the same as the C++ standard doesn't define this.

feliwir commented 4 years ago

Actually that's exactly one of the purposes of reinterpret_cast: From: https://en.cppreference.com/w/cpp/language/reinterpret_cast A pointer can be converted to any integral type large enough to hold all values of its type (e.g. to std::uintptr_t)

So sizeof(std::uintptr_t) or sizeof(std::intptr_t) is guaranteed to match the size of a pointer. So the only change required here is to use one of these types.

std::memcpy is doing something completly different to reinterpret_cast and they are no replacement for each other.

OmniBlade commented 4 years ago

Indeed this is type conversion, not type punning and since the value being "hidden" in the pointer when it is encoded is 32bit at most, there is no risk of loss of information even when the pointer size is 64bit though uintptr_t should probably be used in these types of casts for consistency. When the pointer is decoded a proper pointer is assigned. There are other issues with the save/load system which make it less portable than it could be, but type punning is not one of them in this instance.