Introduction
C++ is a very powerful and versatile tool, but you have to pay for this.
As Bjarne Stroustrup once said:
"C makes it easy to shoot yourself in the foot; C++ makes it harder, but when you do it blows your whole leg off".
That article would teach you how to completely shoot off all your legs (arms, heads and other parts) with the most interesting, unpredictable and exciting ways you can imagine!
Background
In this article, we want to show how it is important to understand the aspects of writing a stable, safe and reliable code and how really easy it is to unintentionally inject vulnerability in it. We hope that would be both interesting and useful to you.
To the Code!
Here is a short snippet of some abstract C++ code. As you can see, that is a code from the Windows DLL (and that point is really important!). Assume that someone is expecting to use that code in some (secure, of course!) solution.
Take time looking at it. Who knows what you can find here? And what in this code could possibly go wrong?
class Finalizer
{
struct Data
{
int i = 0;
char* c = nullptr;
union U
{
long double d;
int i[sizeof(d) / sizeof(int)];
char c [sizeof(i)];
} u = {};
time_t time;
};
struct DataNew;
DataNew* data2 = nullptr;
typedef DataNew* (*SpawnDataNewFunc)();
SpawnDataNewFunc spawnDataNewFunc = nullptr;
typedef Data* (*Func)();
Func func = nullptr;
Finalizer()
{
func = GetProcAddress(OTHER_LIB, "func")
auto data = func();
auto str = data->c;
memset(str, 0, sizeof(str));
data->u.d = 123456.789;
const int i0 = data->u.i[sizeof(long double) - 1U];
spawnDataNewFunc = GetProcAddress(OTHER_LIB, "SpawnDataNewFunc")
data2 = spawnDataNewFunc();
}
~Finalizer()
{
auto data = func();
delete[] data2;
}
};
Finalizer FINALIZER;
HMODULE OTHER_LIB;
std::vector<int>* INTEGERS;
DWORD WINAPI Init(LPVOID lpParam)
{
OleInitialize(nullptr);
ExitThread(0U);
}
BOOL WINAPI DllMain(HINSTANCE hinstDLL, DWORD fdwReason, LPVOID lpvReserved)
{
static std::vector<std::thread::id> THREADS;
switch (fdwReason)
{
case DLL_PROCESS_ATTACH:
CoInitializeEx(nullptr, COINIT_MULTITHREADED);
srand(time(nullptr));
OTHER_LIB = LoadLibrary("B.dll");
if (OTHER_LIB = nullptr)
return FALSE;
CreateThread(nullptr, 0U, &Init, nullptr, 0U, nullptr);
break;
case DLL_PROCESS_DETACH:
CoUninitialize();
OleUninitialize();
{
free(INTEGERS);
const BOOL result = FreeLibrary(OTHER_LIB);
if (!result)
throw new std::runtime_error("Required module was not loaded");
return result;
}
break;
case DLL_THREAD_ATTACH:
THREADS.push_back(std::this_thread::get_id());
break;
case DLL_THREAD_DETACH:
THREADS.pop_back();
break;
}
return TRUE;
}
__declspec(dllexport) int Initialize(std::vector<int> integers, int& c) throw()
{
for (int i : integers)
i *= c;
INTEGERS = new std::vector<int>(integers);
}
int Random()
{
return rand() + rand();
}
__declspec(dllexport) long long int __cdecl _GetInt(int a)
{
return 100 / a <= 0 ? a : a + 1 + Random();
}
Do you find this code quite simple, obvious, absolutely safe and hassle-free? Or maybe you found some problems here? Or maybe you even found a dozen or two?
Well, actually there are more than 43 (yep, forty-three!) potential threats of varying degrees of significance in this code chunk.
Points of Interest
- The
sizeof(d) (where d is a long double) is not necessarily multiple of the sizeof(int)
int i[sizeof(d) / sizeof(int)];
Such a situation is not checked nor handled here. For example, the size of a long double could be 10 on some platforms (which is not true for MS VS compiler, but true for a RAD studio, former C++ Builder).
int can also be of different sizes depending on the platform (well, the code above is for Windows, so, applied specifically to that current situation, the problem is somewhat contrived, but for the portable code, the problem arises).
All that would become a problem if we want a type punning here. By the way, a type punning causes an undefined behaviour due to the C++ language standard (yet still, that is a common practice, because modern compilers usually do define a correct, expected behaviour for that, like a GCC, for example).
By the way, in modern C the type punning is perfectly allowed (you do understand that C and C++ are different languages and that you should not expect to know C if you know C++ and vice verse, do you?)
The solution: Use static_assert to control all those kind of assumptions at the compile time. That would warn you if something with the type's sizes goes wrong:
static_assert(0U == (sizeof(d) % sizeof(int)),
"Size of the bigger type is not multiple of sizes of the smaller type");
time_t is a macro, in Visual Studio, it can refer to 32 (old) or 64 bit (new) integer type:
time_t time;
Accessing that can cause out of border reads/writes or type slicing (corrupting the memory or resulting in reading garbage) if two different binary modules (for example, an executable and a DLL, which it loads) are compiled with the different physical representation of that type.
The solution: Ensure the same strictly sized types are used to share the data between all communicating modules:
int64_t time;
- B.dll (which should be referred by the
OTHER_LIB handle) is not yet loaded at this point, so we will fail to attempt to get an address from it. - static initialization order fiasco (
OTHER_LIB object is used, while it is not yet initialized and contains garbage).
func = GetProcAddress(OTHER_LIB, "func");
FINALIZER is a static object, which is constructed before a call to the DllMain. So in its constructor, we are attempting to use the library, which is loaded later. And the problem worsens because OTHER_LIB static object which is used by the FINALIZER static object is defined later than it in the translation unit, which means it would be initialized (zeroed) later. That means it will simply contain some pseudo-random garbage. Gladly WinAPI should handle that correctly, because with the high probability there will be no module loaded with such handle value, and even if it does exist - it would probably lack the "func" function in it (but if it eventually does, oh boy...)
The solution: The general hint is to avoid using global objects at all, especially complicated ones, especially if they are depending on each other, especially in the DLL. However, if you still need them for some reason, be very careful with their initialization order. To control that order, place all global objects instances (definitions) in the one translation unit in the correct order, to ensure they are initialized properly.
- The previously returned result is not checked before use:
auto data = func();
func is a pointer to the function. It should point to the function from the B.dll. But, because we completely failed all the things in the previous step, it will be nullptr. So attempting to dereference it will lead to something interesting and fascinating like access violation or general protection fault, etc.
The solution: When dealing with the external code (WinAPI in our case), always check the return result of the provided functions. For reliable and fail-safe systems, this rule is still useful even if there are strict contracts that exist for those functions.
- Garbage if compiling with the different alignment/padding settings:
auto str = data->c;
If Data struct (which is used to share information between the communicating modules) has different physical representation through the binary modules, we will end up in the previously mentioned access violation, general protection fault, segmentation-fault, heap corruption, etc. Or we will read garbage. An exact outcome depends on the actual scenario of using that memory. All of that could happen because the struct itself lacks an explicit alignment/padding settings, so in case those global settings were different for those communicating modules when they were compiled, we run into trouble.
The solution: Ensure all shared data structures have strict, explicitly defined and obvious physical representation (fixed-size types, alignment definition, etc.) and/or communicating binaries are compiled with the same alignment/padding settings.
See Also
- Using the size of the pointer instead of the size of an array, which it is pointed:
memset(str, 0, sizeof(str));
That is usually a typo. But things can be complicated when dealing with static polymorphism or using auto keyword (especially when it is overused). I really hope modern compilers are already smart enough to detect such problems during the compilation phase using its internal static code analysis capabilities.
The Solution
- UB when accessing another field, then one which was set
- Possible out of bound access if the size of
long double differs between binary modules:
const int i0 = data->u.i[sizeof(long double) - 1U];
Well, that was already mentioned earlier, so here we just got another point of presence of previously discussed problems.
The solution: Do not access another field, then one which was previously set, unless you are pretty sure your compiler handles that correctly. Ensure sizes of types of shared objects is the same in all communicating modules.
See Also
- Even if the B.dll was correctly loaded and "
func" function is correctly exported and located, B.dll is anyway unloaded at this point (because of the FreeLibrary call in a DllMain/DLL_PROCESS_DETACH callback section), so we will get a crash here:
auto data = func();
Possibly, calling member function using the destroyed polymorphic object or calling the function from an unloaded dynamic library will lead to the pure virtual function call.
The solution: Implement correct finalization routine in the application, ensuring all dynamic libraries finish their work and unloaded in the proper order. Avoid using static objects with complicated logic in the DLL. Avoid performing actions after the DLL finally exits its entry point (and starting to destroy the static objects).
Understand the DLL life cycle:
... other module calls LoadLibrary ...
- construction of the library static objects (should contain only very simple logic, called automatically)
DllMain -> DLL_PROCESS_ATTACH callback event (should contain only very simple logic, called automatically)
[!!] From now other threads of the application can start calling
DllMain -> DLL_THREAD_ATTACH/DLL_THREAD_DETACH in parallel (called automatically, see notes on p. 30)
Those sections can possibly contain some complicated logic (like per thread random seeding) but still beware
- Custom initialization routine (exported by the DLL developer) is called.
(contains all the heavy initialization work, should be manually called by one, who is loading your library)
[your library can create its own threads now and later]
[..] the library performs its main work
- Custom deinitialization routine (exported by the DLL developer) is called.
(Contains all the heavy finalization work, should be manually called by one, who loaded your library.)
[After this point, avoid performing any actions in your library, all previously started library threads should be finished before returning from that function.]
... other module calls FreeLibrary ...
DllMain -> DLL_PROCESS_DETACH (should contain only very simple logic, called automatically)- Destruction of the library static objects (should contain only very simple logic, called automatically)
- Deleting an opaque pointer (the compiler needs to know a complete type to call the destructor, so deleting an object through opaque pointer can result in a memory leak and other problems)
- (assuming the destructor of a
DataNew is virtual) even if the class is correctly exported and imported and we got full information about it, still calling its destructor at this point is a problem - it will likely result in a pure virtual function call (as DataNew type is imported from already unloaded B.dll). And even if it is not (virtual), still, we got a problem here. - If
DataNew class is an abstract polymorphic type and its base class has a pure virtual destructor without a body, there will be a pure virtual function call anyway - UB if allocated using
new and deleting using delete[]
delete[] data2;
In general, you should always be cautious when freeing and deleting objects received from the external modules.
Also, it is a good practice to nullify the pointers on the deleted objects.
The solution is to ensure that:
- when an object is being deleted, its full type (which is pointed by the pointer we deleting by) is known
- all destructors have a body
- the library from which any code is exported is not unloaded too early
- correct form of
new and delete are always used - the pointer pointing to the deleted object(s) is nullified
Additionally Note That
See Also
ExitThread is the preferred method of exiting a thread in C code. In the C++ code, the thread is exited before any destructors can be called or any other automatic cleanup can be performed, so you should return from your thread function:
ExitThread(0U);
The solution: Never use this function manually in the C++ code, but rather just exit normally (by return statement) from the thread function.
- Calling functions that require DLLs other than Kernel32.dll may result in problems that are difficult to diagnose. Calling User, Shell, and COM functions can cause access violation errors because some functions load other system components:
CoInitializeEx(nullptr, COINIT_MULTITHREADED);
The solution - In the DllMain entry point:
- avoid any complicated (de)initialization
- avoid calling functions from the other libraries (or at least be very careful)
- Incorrect initialization of the random seed in a multithreaded environment
- As
time has 1 sec. resolution, any threads in the program that calls time within that period will have the same seed, which could lead to collisions (for example, generating the same pseudo-random temporary file names, same port numbers, etc.). One of the possible solutions is to shuffle (xor) seed's bits with some other pseudo-random values, like the address of any stack or better heap object, more precise time, etc.
srand(time(nullptr));
The solution: MS VS requires that seed should be initialized per thread. Also, using Unix time as a seed gives not enough randomness, prefer to use more advanced seed generation.
See Also
- Can cause a deadlock or a crash (or create dependency loops in the DLL load order):
OTHER_LIB = LoadLibrary("B.dll");
The solution: Do not use LoadLibrary in the DllMain entry point. Any complicated (de)initialization should be done in the specific exported functions like "Init" and "Deint". Your module provides those functions as a result of a contract established between importing and exporting modules. Both parties must strictly enforce the contract.
- Misprint (condition is always
false), incorrect program logic and possible resource leak (since OTHER_LIB is never unloaded if loaded successfully):
if (OTHER_LIB = nullptr)
return FALSE;
Copy assignment operator returns left type reference, i.e., if would check the value of OTHER_LIB (which will be nullptr) and nullptr will be interpreted as false.
The solution: Always use reversed form to avoid such misprints:
if/while (<constant> == <variable/expression>)
- Better use
_beginthread (especially if linked to the static C run-time library) or you can get memory leaks in a call to the ExitThread, DisableThreadLibraryCalls. - DLL notifications are serialized, the entry-point function (
DllMain) should not attempt to create or communicate with other threads or processes (deadlocks may occur):
CreateThread(nullptr, 0U, &Init, nullptr, 0U, nullptr);
- Calling COM functions during termination can cause access violation errors because the corresponding component may already have been unloaded or uninitialized:
CoUninitialize();
- There is no way to control the order in which in-process servers are loaded or unloaded, so do not call
OleInitialize or OleUninitialize from the DllMain function:
OleUninitialize();
See Also
COM Clients and Servers
In-process, Out-of-process, and Remote Servers
- Calling
free on memory block allocated with the new - If the process is terminating (the
lpvReserved parameter is non-NULL), all threads in the process except the current thread either have exited already or have been explicitly terminated by a call to the ExitProcess function, which might leave some process resources such as heaps in an inconsistent state, so it is not safe for the DLL to clean up the resources. Instead, the DLL should allow the operating system to reclaim the memory:
free(INTEGERS);
The solution: Ensure an old C style of dealing with the dynamic memory is not mixed with the modern C++ style. Be very careful when managing the resources in a DllMain entry point.
- Can result in a DLL being used after the system has executed its termination code:
const BOOL result = FreeLibrary(OTHER_LIB);
The solution: Do not call FreeLibrary in the DllMain entry point.
- Will crash current (possibly main) thread:
throw new std::runtime_error("Required module was not loaded");
The solution: Prefer not to throw exceptions in the DllMain entry point. If the DLL could not be loaded correctly for any reason, it should return FALSE. Throwing exceptions during the DLL_PROCESS_DETACH is not only a bad design approach (and almost meaningless) but also could possibly lead to the problems during the deinitialization stage.
In any case, always be very careful throwing exceptions outside of the DLL. Any complicated objects (like classes of the standard library) may have a different physical representation (and even logic) in some cases, for example, if two binary modules are compiled with the different (incompatible) versions of the runtime library.
Prefer to exchange only simple data types (with fixed sizes and determined representation) between modules.
Also, remember, that exiting or terminating the main thread will automatically terminate all the others (which would not have a chance to be finished correctly, so they can corrupt the memory, leaving mutexes, heaps and other objects in the unpredictable, inconsistent state, and also those threads would be already dead at the time when the static objects will start their own deconstruction, so do not attempt to wait for a thread here).
See Also
- Can throw an exception (
std::bad_alloc, for example), which is not caught here:
THREADS.push_back(std::this_thread::get_id());
Since DLL_THREAD_ATTACH section is invoked from some unknown external code, do not expect the correct behaviour here.
The solution: Enclose with the try/catch those instructions, which could possibly throw exceptions that can't be expected to be handled correctly (especially if they go out of the DLL).
See Also
- UB if there were threads presented before this DLL was loaded:
THREADS.pop_back();
Existing threads (including that one which is actually loading the DLL) do not call the entry-point function of the newly loaded DLL (so they are not registered in the THREADS vector during DLL_THREAD_ATTACH event), while they still call it with DLL_THREAD_DETACH on finishing.
Which means a consideration that a number of calls to the DLL_THREAD_ATTACH and DLL_THREAD_DETACH are always equal is wrong, those making any logic depending on it dangerous.
- Compiler dependent
int size, better use C++11 fixed-size integer - Passing complicated object between modules (can cause a crash if the are compiled with the different runtimes: release/debug, different versions, etc.)
- Accessing object
c by its virtual address (which is shared between modules) can cause problems, if pointers are threated differently in those modules (for example, if the modules are linked with the different [/LARGEADDRESSAWARE] options)
__declspec(dllexport) int Initialize(std::vector<int> integers, int& c) throw()
See Also
And Also...
And Finally...
Wait, did I forgot something? Surely I did! :)
Because pointers are, in fact, much more complicated stuff than people usually think about them. I am pretty sure you can add something important in the comments (maybe something about the difference between pointer to object and a pointer to the function, that perhaps not all the bits in a pointer value can be used to form an address and so on).
for (int i : integers)
i *= c;
Mistake: Original items in the container would not change, need to use a reference (prefer to use two types of references: 1 and 2:)
- Exception can be thrown inside the function:
INTEGERS = new std::vector<int>(integers);
However, that function's throw specification is empty:
__declspec(dllexport) int Initialize(std::vector<int> integers, int& c) throw()
std::unexpected is called by the C++ runtime when a dynamic exception specification is violated: an exception is thrown from a function whose exception specification forbids exceptions of this type.
The solution: Use try/catch (especially when allocating resources, especially in the DLL) or use nothrow form. In any case, do not expect infinite resources.
See Also
Problem 1: Forming such a "more random" value is incorrect. As the сentral limit theorem states, a sum of the independent random variables tends toward a normal distribution (even if the original variables themselves are not normally distributed).
Problem 2: Possible integer overflow (which is UB for signed integers):
return rand() + rand();
When dealing with such things like randomization, encryption, etc., beware of using some homemade "solutions". If you lacking a specific math education and knowledge, heavy experience with those concepts, chances are high that you will simply outsmart yourself, making things worse.
- The exported function name will be decorated (mangled), to prevent this use
extern "C" - Names started from '_' are implicitly forbidden for C++, as that naming style is reserved for STL
__declspec(dllexport) long long int __cdecl _GetInt(int a)
Multiple problems (and their possible solutions):
rand is not thread-safe, need to use rand_r/rand_s insteadrand is obsolete, consider to use modern C++11 <random>- The seed for
rand was not possibly initialized for this thread (MS VS requires per-thread initialization) - It is not crypt safe, use specific OS API or some portable solution (Libsodium/
randombytes_buf, OpenSSL/RAND_bytes, etc.) - Possible division by zero: can cause current thread termination
- Operators priority (use brackets) and/or sequence points
- Possible integer overflow:
return 100 / a <= 0 ? a : a + 1 + Random();
See Also
And Also...
That's Not All! We Have Even More Intriguing Code for You ;)
Imagine you have some important content in memory (user password, for example). Surely you don't want to keep it in memory for a long time (increasing the probability someone could read it from here).
A naive approach to achieve that would look like that:
bool login(char* const userNameBuf, const size_t userNameBufSize,
char* const pwdBuf, const size_t pwdBufSize) throw()
{
if (nullptr == userNameBuf || '\0' == *userNameBuf || nullptr == pwdBuf)
return false;
const bool result = doLoginInternall(userNameBuf, pwdBuf);
memset(userNameBuf, 0, userNameBufSize);
memset(pwdBuf, 0, pwdBufSize);
}
Well, that, of course, would not work. So, what to do then?
Wrong "solution" #1: If memset isn't working, let's do that manually!
void clearMemory(char* const memBuf, const size_t memBufSize) throw()
{
if (!memBuf || memBufSize < 1U)
return;
for (size_t idx = 0U; idx < memBufSize; ++idx)
memBuf[idx] = '\0';
}
And there is no reason why the modern compiler can't optimize that.
Btw, the memset function would be compiler intrinsic if they are enabled. That changes nothing in the current context, just an interesting thing to know.
See Also
Wrong "solution" #2: Trying to "improve" the previous "solution" by playing with the volatile keyword:
void clearMemory(volatile char* const volatile memBuf, const volatile size_t memBufSize) throw()
{
if (!memBuf || memBufSize < 1U)
return;
for (volatile size_t idx = 0U; idx < memBufSize; ++idx)
memBuf[idx] = '\0';
*(volatile char*)memBuf = *(volatile char*)memBuf;
}
Would that work? Well, it might. Probably. For example, such an approach is used in the MS VS RtlSecureZeroMemory (you can check its actual implementation in the Windows SDK sources). However, this is heavily compiler-dependent.
See Also
Wrong "solution" #3: Try to use wrong OS API (like RtlZeroMemory) or even STL (like std::fill, std::for_each) instead of the CRT or homemade code:
RtlZeroMemory(memBuf, memBufSize);
And there are even more possibly wrong solutions!
And, Finally, How to Really Fix That?
- Use a specific OS API function, like
RtlSecureZeroMemory for Windows - C11 function
memset_s is also suitable for that purpose:
Quote:
Unlike memset, any call to the memset_s function shall be evaluated strictly according to the rules of the abstract machine.
Also, we can prevent the compiler from optimizing the code out by outputting (to the file, console or another stream) the variable value, but this way obviously is not very useful.
To Be Continued...
That is, of course, is not a complete list of all the possible troubles you can encounter writing applications using C/C++.
There are also such wonderful things like livelocks, race conditions (for example, caused by incorrect implementations of a none-blocking algorithm, ABA problems, improperly changing multiple atomics at once, thread-unsafe reference counters, incorrect implementations of a double-checking lock pattern and so on), objects slicing, loss of arithmetic precision (due to rounding or numerically unstable algorithms, for example, summation of many doubles without first sorting them), threads and GDI objects, volatile vs atomic, incorrect using of an integer literals (603 vs 0603), time-of-check to time-of-use, lambdas which outlives their reference captured objects, incorrect printf-family functions formatters, incorrectly sharing data between two devices with the different endianness (for example, through the network), bitfield details, confusing C++ exceptions and SEH, performing incorrect stack allocations, disabling ASLR, possible backdoors in API, confusing sizeof vs _countof, not using correct memory locking (also not that suspend mode on laptops and some desktop computers will save a copy of the system's RAM to disk, some architecture surprises, regardless of memory locks), stack corruptions, etc. etc. etc.
Want to add more? Share your own interesting materials in the comments!
Want to Know More?
There are a bunch of some other helpful external links we wish to present you. You can refer to those materials to extend your knowledge far further. God bless those wonderful authors across the internet that bring us so many exciting articles to read!
P. S.
When this article was actually finished and ready to be published, browsing up the internet for additional information to add here, this amazing commentary was found:
History
13th August, 2019 - Added more useful links:
