In Visual Studio version 16.8 Preview 3,  we have added a few safety rules to C++ Code Analysis that can find some common mistakes, which can lead to bugs ranging from simple broken features to costly security vulnerabilities. These new rules are developed around issues discovered in production software via security reviews and incidents requiring costly servicing. Every shipping piece of software in Microsoft runs these rules as part of security and compliance requirements. 

The first part of this blog series, New Safety Rules in C++ Code Analysis, introduced new rules related to the misuse of VARIANT and its sibling types – such as VARIANTARG, or PROPVARIANT. 

This second part of the series introduces new rules about “use of enumerations as index” and “use of Boolean as HRESULT”. To help with these new rules, we have built two code analysis extensions, called EnumIndex and HResultCheck that detect violations of these new rules in code. 

Using enum as index 

An enumeration or enum is a user-defined integral type that consists of an optional set of named integral constants that are known as enumerators (also called enumeration-constants). Usually, an enumeration provides context to describe a range of values (called enumerators) which are represented as named constants. 

An enum can be made scoped by specifying class or struct keyword after the enum keyword, for example: 

enum class Suit { Diamonds, Hearts, Clubs, Spades };

Without the class or struct keyword, an enum becomes unscoped. 

Using /std:c++17, an enum (regular or scoped) can be defined with an explicit underlying type and no enumerators, which in effect introduces a new integral type that has no implicit conversion to any other type. 

Unscoped enumerators can be implicitly converted to int. Scoped enumerators cannot be implicitly converted to int. A cast is required to convert a scoped enumerator to int. Likewise, a cast is required to convert an int to a scoped or unscoped enumerator. 

The fact that an enumeration is an integral type that usually consists of a finite set of named constant values (enumerators) which can be converted implicitly or explicitly to int makes it very common to use enumerators as index values. For example:

const auto& colorInfo = ColorTable[color];

You will find lots of discussions online on using enum values as array indices. It really makes sense in many situations. 

Frequently, when developers use enumerators of an enum type as indices for an array, they know that the enumerators of the enum type have values starting from zero to a known maximum value, with an increment of one and with no gap between any pair of consecutive enumerators. Thus, most of developers think checking the enumerator value received against the known maximum value would ensure validity of it. 

However, using enumerators as array indices is not very safe. Unfortunately, it seems that there are not many discussions on why it can be dangerous. 

Let’s look at an example. Consider the following enum and a table of function pointers for which we want to use the enum value as the index: 

// MyHeader.h 
 
#pragma once 
 
#include <iostream> 
 
typedef int (*FP)(); 
 
enum FunctionId 
{ 
    Function1, 
    Function2, 
    Function3, 
    FunctionCount 
}; 
 
template <int val> 
int GetValue() { return val; }; 
 
int DoNotCallMe() 
{ 
    std::cout << "This shouldn't be called!\n"; 
    return -1; 
} 
 
FP fp = DoNotCallMe; 
 
FP Functions[] 
{ 
    GetValue<0>, 
    GetValue<1>, 
    GetValue<2> 
};

Now, in a source file, let’s define a function to select a function from the table, using an enumerator of the enum as the index for the function pointer table:

#include "MyHeader.h" 
 
FP GetFunction(FunctionId funcId) 
{ 
    if (funcId < FunctionId::FunctionCount) 
        return Functions[funcId]; 
    return nullptr; 
} 

Neat, isn’t it? To protect from rogue or faulty callers, I check the enumerator value against the known maximum value for FunctionId, so that it doesn’t cause the function to access the table beyond its bound. I know the enumerators of FunctionId enum type will start from zero, incremented by one, and end at FunctionId::FunctionCount – 1, FunctionCount being the last enumerator in the enum. 

Let’s continue to add more code that uses this function. Our customer code will have integer value as the selector of a function, and want us to return an integer value through the function: 

int GetValue(int funcIdx) 
{ 
    const auto fp = GetFunction(static_cast<FunctionId>(funcIdx)); 
    return fp ? fp() : -1; 
}

As explained above, I needed a cast to convert the integer value for the function table index to the enum type to pass to GetFunction. That will make sure that the int value is properly converted to an enumerator of the FunctionId enum. So far, so good, I hope. 

Now, let’s consider a function that calls GetValue to get a value through a function: 

int main() 
{ 
    return GetValue(-1); 
}

Where did -1 come from? For this discussion, that is not important. Let’s assume it was from user’s input. Anyway, this obviously seems wrong. However, I didn’t get any hint from compiler on potential problem with this call, even with /Wall. In fact, nothing is “wrong” considering the types involved and how they are used. But we know this is wrong. Does GetFunction really protect itself from this call? A short answer is – No. 

Problems are, that you can cast any int value to an enum type, and that an enum’s underlying type defaults to int – signed int. For a signed value, if you check the upper bound but not its lower bound, you end up allowing negative values. In the above example, it ended up calling the dangerous DoNotCallMe function, that happens to be right before the function pointer table. In real life, this kind of bug can lead to an exploitable security vulnerability. 

It is less likely someone checks for the lower bound but forgets to check the upper bound. However, that can also cause the same problem, by allowing access beyond the array bound. 

Just for fun, running the above example produces the following output for me:

This shouldn't be called! 
C:\Temp\Sample.exe (process 9748) exited with code -1.

EnumIndex Rules

The EnumIndex extension finds defects like the one shown above, and reports them through the following warnings: 

• C33010: Unchecked lower bound for enum ‘enum‘ used as index.  

• C33011: Unchecked upper bound for enum ‘enum‘ used as index.  

Warning C33010

This warning is triggered for an enum that is used as an index into an array, if the upper bound is checked for its value, but not the lower bound. 

Here is a simplified example:  

typedef void (*PFN)(); 
 
enum class Index 
{ 
    Zero, 
    One, 
    Two, 
    Three, 
    Max 
}; 
 
void foo(Index idx, PFN(&functions)[5]) 
{ 
    if (idx > Index::Max) 
        return; 
 
    auto pfn = functions[static_cast<int>(idx)];    // C33010 
    if (pfn != nullptr) 
        (*pfn)(); 
    // ...... 
}

These warnings are corrected by checking the index value for lower bound as well: 

typedef void (*PFN)(); 
 
enum class Index 
{ 
    Zero, 
    One, 
    Two, 
    Three, 
    Max 
}; 
 
void foo(Index idx, PFN(&functions)[5]) 
{ 
    if (idx < Index::Zero || idx > Index::Max) 
        return; 
 
    auto pfn = functions[static_cast<int>(idx)];    // OK 
    if (pfn != nullptr) 
        (*pfn)(); 
    // ...... 
}

Warning C33011 

This warning is triggered for an enum that is used as an index into an array, if the lower bound is checked for its value, but not the upper bound. 

Here is a simplified example:  

typedef void (*PFN)(); 
 
enum class Index 
{ 
    Zero, 
    One, 
    Two, 
    Three, 
    Max 
}; 
 
void foo(Index idx, PFN(&functions)[5]) 
{ 
    if (idx < Index::Zero) 
        return; 
 
    auto pfn = functions[static_cast<int>(idx)];    // C33011 
    if (pfn != nullptr) 
        (*pfn)(); 
    // ...... 
}

These warnings are corrected by checking the index value for upper bound as well: 

typedef void (*PFN)(); 
 
enum class Index 
{ 
    Zero, 
    One, 
    Two, 
    Three, 
    Max 
}; 
 
void foo(Index idx, PFN(&functions)[5]) 
{ 
    if (idx < Index::Zero || idx > Index::Max) 
        return; 
 
    auto pfn = functions[static_cast<int>(idx)];    // OK 
    if (pfn != nullptr) 
        (*pfn)(); 
    // ...... 
}

Enabling EnumIndex rules in Visual Studio 

You can enable EnumIndex rules in Visual Studio as follows by selecting different ruleset for your project:  

Rule ID	Extension	Native Minimum Rules	Native Recommended Rules	All Rules
C33010	EnumIndex	X	X	X
C33011	EnumIndex		X	X

  

Using Boolean as HRESULT 

While it may not be intentional, we have seen code where Boolean values were used as HRESULT values, and vice versa. C/C++ allow implicit conversions between them, and compilers wouldn’t warn about these implicit conversions. However, a Boolean value and an HRESULT have different semantics, and may not be used interchangeably. 

That is why there is already a rule against this misuse. Consider the following example: 

#include <windows.h> 
BOOL IsEqual(REFGUID, REFGUID); 
 
HRESULT foo(REFGUID riid1, REFGUID riid2) 
{ 
    return IsEqual(riid1, riid2); 
}

The intention of foo() is to compare the two values and return S_OK when they are equal. However, it will return S_FALSE if the values are equal, and S_OK if the values are different. This is quite the opposite to the intended behavior. However, this code will likely compile just fine without getting a warning about this potential defect. Fortunately, C++ Code Analysis can detect this and will report a C6216 warning, which is a general warning about implicit cast of Boolean value to HRESULT. 

Among various potential misuses of Boolean and HRESULT values, we learned that one specific scenario occurs more often than others, and leads to more obvious bugs. We have created an additional extension to cover this very scenario – HResultCheck. 

HResult Rules

The HResultCheck extension finds where a C style BOOL FALSE is returned from a function as an HRESULT value, leading to returning S_OK when the intention is presumably returning a failure result: 

• C33020: Likely incorrect HRESULT usage detected. 

• C33022: Potentially incorrect HRESULT usage detected (low confidence).  

Warning C33020 

This is a high-confidence warning indicating that HRESULT-returning function returns FALSE. In many cases, developers consider FALSE as a failure value, and return it from a function with the intention of indicating failure. However, the value of FALSE is 0. When interpreted as an HRESULT value, this value becomes S_OK, representing success. 

Here is a simplified example: 

#include <Windows.h> 
 
HRESULT foo() 
{ 
    // ...... 
    return FALSE; // C33020 
}

This can be fixed returning a proper HRESULT value: 

#include <Windows.h> 
 
HRESULT foo() 
{ 
    // ...... 
    return E_FAIL; // OK 
}

Warning C33022

This is low-confidence warning for a function that returns HRESULT, if there is FALSE somewhere along the line that eventually returns it. 

Here is a simplified example: 

#include <Windows.h> 
 
#define RETURN_FAILURE(x) do { *res = x; return FALSE; } while(false); 
 
HRESULT test04(BOOL* res) 
{ 
    // ... 
    RETURN_FAILURE(FALSE); 
    // ... 
    return S_OK; 
}

This can be fixed by using a proper HRESULT value: 

#define RETURN_FAILURE(x) do { *res = x; return E_FAIL; } while(false); 
 
HRESULT test04(BOOL* res) 
{ 
    // ... 
    RETURN_FAILURE(FALSE); 
    // ... 
    return S_OK; 
}

Enabling HResult rules in Visual Studio

You can enable HResult rules in Visual Studio as follows by selecting different ruleset for your project:  

Rule ID	Extension	Native Minimum Rules	Native Recommended Rules	All Rules
C33020	HResultCheck	X	X	X
C33022	HResultCheck			X

 

Give us your feedback

Check out these newly added rules and let us know how they help you write safer C++. Stay tuned as we add more safety rules in future releases of Visual Studio.  

Download Visual Studio 2019 version 16.8 Preview 3 today and give it a try. We would love to hear from you to help us prioritize and build the right features for you. We can be reached via the comments below, Developer Community, and Twitter (@VisualC). The best way to file a bug or suggest a feature is via Developer Community.