C++ Code Smells

A recent security bulletin from Microsoft highlights the dangers of certain constructs in C++. These constructs constitute a set of code smells for C++. In this post, I’ll describe what I consider to be C++ code smells and how to deal with them.

void *

Typeless pointers, or pointers to void — void *, are vestigal leftovers from C++’s compatability with C. If you are calling into some API that uses typeless pointers, you should immediately wrap that typeless construct into a strongly typed construct. Let’s consider an example from Direct3D: locking an index buffer gives you a typeless pointer to the locked data. Using a smart resource lock class hides the typeless pointer inside the resource lock.

Every pointer in C++ can be implicitly converted to a pointer to void. Any time you see a cast to void *, you can safely remove the cast because it is redundant. In the Microsoft security bulletin, the underlying cause of the error was hidden by the programmer inserting an explicit cast to pointer to void. Chances are that the code was originally written without the cast and instead of understanding the compiler error message, the programmer just inserted the cast and forced the compiler to accept incorrect code.

Don’t write code that takes void * arguments, returns values as void * or contains casts to void *. Don’t write methods or functions that take no arguments as taking an argument list that is (void). Because the syntax of early C functions didn’t require you to declare the types of the arguments, an empty parameter list in C is ambiguous. It could represent an empty parameter list, or it could represent merely that the declared identifier was a function with an unknown argument list. In C++, the empty argument list () is never ambiguous and always represents an a method or function that takes no arguments.

If you have to use someone else’s API that uses void *, wrap that API in some type safe construct. The Win32 API and many COM objects are callable from C++, but they are essentially C style APIs (in the case of Win32) or have to rely on binary compatability between languages (in the case of COM objects) and they use void *. Wrap those constructs up in something that is typesafe C++ and you can use them safely.

catch (...)

Exceptions are great for transferring control from the point of error detection to the point of error handling and handing some context about the error between the two points. C++ lets you throw any value you like, including plain data types like int, float, enumerations, etc. The problem with these plain data types is that they don’t participate in an inheritance hierarchy, so it is difficult to write a single catch statement that handles both class based exceptions and these plain data types.

The catch (...) construct is meant to alleviate the need for duplicate exception handlers by allowing you to write a single exception handler that catches any exception. This sounds great, but there’s a problem.

On Windows, its possible for catch (...) to catch things that aren’t generated by throw statements. In particular, it can catch an access violation caused by an attempt to dereference an invalid pointer. Sometimes, this can be a good thing. For instance, it lets you write a top-level catch handler in your main routine that creates a minidump when an access violation occurs and uploads that minidump to a customer support site for analysis.

The problem occurs when there are catch (...) phrases sprinkled throughout the body of your application. Unless the exception handlers rethrow the caught exception, this can cause access violations (or other errors) to be suppressed. Error suppression is not error handling. In particular, suppression of access violations can lead the user to believe that the program is functioning correctly when in fact things have gone very, very wrong.

If you have a catch (...) handler because you want to perform identical cleanup regardless of the exception that is thrown, then make sure that the handler always rethrows the exception after the cleanup is performed. An even better alternative would be to use the RAII idiom to perform the cleanup, eliminating the need for the whole trycatch construct in the first place.

A catch (...) handler that does not rethrow the exception indicates code that is suppressing errors rather than handling them. Such constructs should be modified to rethrow the caught exception to a point where the exception can be properly handled. An alternative is to modify the catch statement to catch specific exceptions that can be handled at that point in the code.

reinterpret_cast<T>

The reinterpret_cast operator is inherently unsafe and it is comparable to C style casts that are also unsafe. The real problem with reinterpret_cast is that it suppresses all of the compiler’s ability to help you identify incorrect code with the type system. You should prefer static_cast, const_cast and dynamic_cast over reinterpret_cast. If you find yourself needing reinterpret_cast in your code, its probably because you’ve written incorrect code and need reinterpret_cast to force the compiler to ignore its type checking and just “jam it in there” anyway.

(T) expr

C-style casts where the type of expr is cast to another type T can also be inherently unsafe where pointers are concerned. The problem with a C-style cast and pointers is that the intent of the programmer isn’t made clear from the cast. Was it an attempt to remove a const or volatile qualifier? Was it an attempt to downcast a pointer to a base class to a derived class? This ambiguity inherent in a C-style cast is why the new casting operators were created for C++. Depending on the intent, such C-style casts on pointers should be replaced with the appropriate C++ casting operator.

C-style casts can also be used to convert values from one type to another, such as when a float is cast to an int. Such casts are not unsafe, but there are alternatives in C++ that are less ambiguous about the intent. The static_cast operator can be used to apply type conversions, including implicit and user-defined conversions. Its also possible to use constructor syntax for the conversion, making it more explicit that you are attempting to construct one type from the value of another type. For instance, the following code uses constructor syntax to create an enum from an integer value:

enum WindowGravity { Left = 0, Right, Top, Bottom };
WindowGravity gravity = WindowGravity(2); // Top

C++ FAQ Lite

If these issues with C++ are just being brought to your attention for the first time, you might want to read the C++ FAQ Lite which covers quite a bit of ground. Bjarne Stroustrup also discusses the casting operators in C++ and how they eliminate ambiguities present in the C-style casts in The C++ Programming Language.

[Updated to clarify discussion of reinterpret_cast]

7 Responses to “C++ Code Smells”

  1. Me Says:

    Not completely accurate. Not all pointers can be converted to a void*. Function pointers cannot be; member pointers cannot be.

    • legalize Says:

      True. g++ gives an error on both of those. gcc allows the function pointer to be assigned without warning or error. VS.NET 2008 (VC9) allows the function pointer assignment without warning or error, even at the highest warning level. I’ve filed a bug on that.

  2. legalize Says:

    The bug report came back saying that for legacy compatability reasons assigning a pointer to function into a pointer to void is only reported as an error when the /Za flag is used. See the MSDN documentation.

  3. Emil Dotchevski Says:

    A nit, reinterpret_cast isn’t equivalent to C-style cast. C-style casts will do the necessary pointer arithmetic to convert between two types that are related, while reinterpret_cast simply treats the pointer as something else.

    Also, catch(…) and eating the exception without rethrowing is appropriate (required?) in destructors that call functions that may throw.

  4. legalize Says:

    Thanks, I clarified the language about reinterpret_cast. You are correct, it isn’t identical. I wanted to talk about how they are both intrinsicly unsafe for C++ code, not try to make them appear identical.

    And yes, catch (...) is sometimes required. This post was about code smells: things that may indicate a problem in your code when they are present. To clarify what I mean by code smell, I’ve linked to its wikipedia entry in the opening paragraph.

    The presence of a code smell doesn’t guarantee the presence of a problem, but more often than not when I’ve seen these kinds of things in C++ code, they are a source of bugs and deserve more scrutiny.

  5. “Effective C++”, 3rd edition by Scott Meyers « Legalize Adulthood! Says:

    [...] 27: Minimize casting once again reminds us once again how casts are a code smell for C++ and are best avoided. Why, just today I was reviewing some code in a product where I work and found [...]

  6. Engineering Notebooks: High Level Thoughts and TDD To Do Lists « Legalize Adulthood! Says:

    [...] likely to notice something else that also needs doing. You may also encounter something smelly about the code you’re modifying that could be improved later. The idea of the “To [...]


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: