{"id":26998,"date":"2020-10-28T15:00:51","date_gmt":"2020-10-28T15:00:51","guid":{"rendered":"https:\/\/devblogs.microsoft.com\/cppblog\/?p=26998"},"modified":"2020-10-28T11:11:04","modified_gmt":"2020-10-28T11:11:04","slug":"even-more-new-safety-rules-in-c-code-analysis","status":"publish","type":"post","link":"https:\/\/devblogs.microsoft.com\/cppblog\/even-more-new-safety-rules-in-c-code-analysis\/","title":{"rendered":"Even More New Safety Rules in C++ Code Analysis"},"content":{"rendered":"

In\u202f<\/span>Visual Studio version 16.8\u202fPreview 3<\/span><\/a>,\u202f\u202fwe<\/span>\u202f<\/span>have added<\/span>\u202fa\u202ffew\u202fsafety rules to C++ Code Analysis\u202fthat can\u202ffind\u202fsome common mistakes, which can lead to\u202fbugs ranging\u202ffrom simple broken features to\u202fcostly security vulnerabilities.\u202fThese new rules are developed around issues discovered in\u202f<\/span>production<\/span>\u202fsoftware via\u202fsecurity reviews and incidents\u202frequiring\u202fcostly\u202f<\/span>servicing<\/span>.\u202fEvery shipping piece of software in Microsoft runs these rules as part of security and compliance requirements.<\/span>\u00a0<\/span><\/p>\n

The first part of this blog series,\u00a0<\/span>New Safety Rules in C++ Code Analysis<\/span><\/a>, introduced<\/span>\u00a0new rules related<\/span>\u00a0to<\/span>\u00a0<\/span>the\u00a0<\/span>misuse<\/span>\u00a0of<\/span>\u00a0<\/span>VARIANT<\/span><\/a><\/code>\u202f<\/span>and its sibling types \u2013 such as\u202f<\/span>VARIANTARG<\/span><\/code>, or\u202f<\/code><\/span>PROPVARIANT<\/span><\/code>.<\/span>\u00a0<\/span><\/p>\n

This second\u00a0<\/span>part\u00a0<\/span>of the series\u00a0<\/span>introduce<\/span>s<\/span>\u00a0new rules about \u201cuse of enumerations as index\u201d and \u201cuse of Boolean as HRESULT\u201d.\u00a0<\/span>To help with the<\/span>se<\/span>\u00a0new rules,\u202fwe\u202fhave built\u00a0<\/span>two\u00a0<\/span>code analysis extension<\/span>s<\/span>, called\u202f<\/span>EnumIndex<\/span><\/code>\u00a0<\/code>and\u00a0<\/span>HResultCheck<\/span><\/code>\u00a0<\/code>that\u202fdetect violations of these new rules\u202fin code.<\/span>\u00a0<\/span><\/p>\n

Using\u00a0enum\u00a0as index\u00a0<\/span><\/h2>\n

A<\/span>n<\/span>\u00a0<\/span>enumerat<\/span>i<\/span>on<\/span><\/a>\u00a0or\u00a0<\/span>enum<\/span>\u00a0<\/span>is\u00a0<\/span>a user-defined\u00a0<\/span>integral\u00a0<\/span>type that consists of a<\/span>n optional<\/span>\u00a0set of named integral constants that are known as enumerators<\/span>\u00a0(also called enumeration-constants)<\/span>.<\/span>\u00a0<\/span>Usually, a<\/span>n enumeration provides context to describe a range of\u00a0<\/span>values <\/span>(<\/span>called enumerators)<\/span>\u00a0which are represented as named\u00a0<\/span>constants<\/span>.<\/span>\u00a0<\/span><\/p>\n

An\u00a0<\/span>enum<\/span>\u00a0can be made scoped by specifying class or\u00a0<\/span>struct<\/span>\u00a0keyword after the\u00a0<\/span>enum<\/span><\/code>\u00a0keyword, for example:<\/span>\u00a0<\/span><\/p>\n

enum\u00a0class\u00a0Suit\u00a0{\u00a0Diamonds, Hearts, Clubs,\u00a0Spades\u00a0};<\/pre>\n
Without the class<\/code> or struct<\/code> keyword, an\u00a0<\/span>enum<\/span>\u00a0becomes\u00a0<\/span>unscoped<\/span>.<\/span>\u00a0<\/span><\/p>\n
Using\u00a0<\/span>\/std:c++17<\/span><\/a>, an\u00a0<\/span>enum<\/span>\u00a0(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.<\/span>\u00a0<\/span><\/p>\n
Unscoped<\/span>\u00a0<\/span>enumerators<\/span>\u00a0can be implicitly converted to int<\/code><\/span>.\u00a0<\/span>Scoped enumerators cannot be implicitly converted<\/span>\u00a0to int<\/code>. A cast is required to convert\u00a0<\/span>a scoped enumerator to int. Likewise, a<\/span>\u00a0cast is required to convert an int<\/code>\u00a0<\/span>to<\/span>\u00a0a scoped or\u00a0<\/span>unscoped<\/span>\u00a0enumerator.<\/span>\u00a0<\/span><\/p>\n
The fact that\u00a0<\/span>an\u00a0<\/span>enum<\/span>eration<\/span>\u00a0is an integral type that usually\u00a0<\/span>consist<\/span>s<\/span>\u00a0of\u00a0<\/span>a finite set<\/span>\u00a0of named constant values (enumerators)<\/span>\u00a0which\u00a0<\/span>can be\u00a0<\/span>converted implicitly or explicitly to int<\/code> makes it very common to use enumerators as index values<\/span>. For example:<\/span><\/p>\n
const\u00a0auto&\u00a0colorInfo\u00a0=\u00a0ColorTable[color];<\/pre>\n
You will find lots of discussions online on using\u00a0<\/span>enum<\/span>\u00a0values as array indices. It really makes sense in many situations.<\/span>\u00a0<\/span><\/p>\n
F<\/span>requently<\/span>,\u00a0<\/span>when\u00a0<\/span>developers\u00a0<\/span>use enumerators of an\u00a0<\/span>enum<\/span>\u00a0type as indices for an array, they know that the enumerators of the\u00a0<\/span>enum<\/span>\u00a0type 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<\/span>.<\/span>\u00a0Thus,\u00a0<\/span>most of<\/span>\u00a0developers think checking the enumerator value received against the known maximum value would ensure validity of it.<\/span>\u00a0<\/span><\/p>\n
However, using\u00a0<\/span>enumerators<\/span>\u00a0as array indices is not very safe.\u00a0<\/span>Unfortunately, it seems that\u00a0<\/span>there are not many discussions on why it can be dangerous.<\/span>\u00a0<\/span><\/p>\n
Let\u2019s look at an example.<\/span>\u00a0Consider the following\u00a0<\/span>enum<\/span>\u00a0and\u00a0<\/span>a\u00a0<\/span>table of function pointers<\/span>\u00a0for which we want to use the\u00a0<\/span>enum<\/span>\u00a0value as the index<\/span>:<\/span>\u00a0<\/span><\/p>\n
\/\/ MyHeader.h \r\n \r\n#pragma once \r\n \r\n#include <iostream> \r\n \r\ntypedef int (*FP)(); \r\n \r\nenum FunctionId \r\n{ \r\n    Function1, \r\n    Function2, \r\n    Function3, \r\n    FunctionCount \r\n}; \r\n \r\ntemplate <int val> \r\nint GetValue() { return val; }; \r\n \r\nint DoNotCallMe() \r\n{ \r\n    std::cout << \"This shouldn't be called!\\n\"; \r\n    return -1; \r\n} \r\n \r\nFP fp = DoNotCallMe; \r\n \r\nFP Functions[] \r\n{ \r\n    GetValue<0>, \r\n    GetValue<1>, \r\n    GetValue<2> \r\n};<\/pre>\n
Now, in\u00a0<\/span>a\u00a0<\/span>source file, let\u2019s define a function to select a function from the table, using\u00a0<\/span>an<\/span>\u00a0<\/span>enumerator of the\u00a0<\/span>enum<\/span>\u00a0as the index<\/span>\u00a0for the function pointer table<\/span>:<\/span><\/p>\n
#include \"MyHeader.h\" \r\n \r\nFP GetFunction(FunctionId funcId) \r\n{ \r\n    if (funcId < FunctionId::FunctionCount) \r\n        return Functions[funcId]; \r\n    return nullptr; \r\n}\u00a0<\/span><\/pre>\n
Neat, isn\u2019t it?\u00a0<\/span>T<\/span>o\u00a0<\/span>protect from rogue or faulty callers,\u00a0<\/span>I check the\u00a0<\/span>enumerator\u00a0<\/span>value\u00a0<\/span>against the known maximum value for\u00a0<\/span>FunctionId<\/span><\/code>, so<\/span>\u00a0that it doesn\u2019t cause\u00a0<\/span>the function\u00a0<\/span>to access the table beyond its bound. I know the\u00a0<\/span>enumerators of\u00a0<\/span>FunctionId<\/span><\/code>\u00a0<\/span>enum<\/span>\u00a0type<\/span>\u00a0will start from zero<\/span>, incremented by one,<\/span>\u00a0and end at\u00a0<\/span>FunctionId<\/span>::<\/span>FunctionCount<\/span><\/code>\u00a0\u2013 1<\/code>,\u00a0<\/span>FunctionCount<\/span><\/code>\u00a0being the last enumerator in the\u00a0<\/span>enum<\/span>.<\/span>\u00a0<\/span><\/p>\n
Let\u2019s continue to add more code that uses this function<\/span>. Our customer code will have integer value as the selector of a function, and want us to return an integer value through the function<\/span>:<\/span>\u00a0<\/span><\/p>\n
int GetValue(int funcIdx) \r\n{ \r\n    const auto fp = GetFunction(static_cast<FunctionId>(funcIdx)); \r\n    return fp ? fp() : -1; \r\n}<\/pre>\n
As\u00a0<\/span>explained<\/span>\u00a0above, I needed\u00a0<\/span>a\u00a0<\/span>cast\u00a0<\/span>to convert the\u00a0<\/span>integer value\u00a0<\/span>for the function table index\u00a0<\/span>to the\u00a0<\/span>enum<\/span>\u00a0type<\/span>\u00a0to pass to\u00a0<\/span>GetFunction<\/span><\/code>. That will make sure that the\u00a0<\/span>int<\/span><\/code>\u00a0value is properly converted to\u00a0<\/span>an enumerator of the\u00a0<\/span>FunctionId<\/span><\/code>\u00a0<\/span>enum<\/span>.<\/span>\u00a0So far, so good,\u00a0<\/span>I hope.<\/span>\u00a0<\/span><\/p>\n
Now, let\u2019s consider a function that\u00a0<\/span>calls<\/span>\u00a0<\/span>GetValue<\/span><\/code>\u00a0<\/span>to get\u00a0<\/span>a value<\/span>\u00a0through a function<\/span>:<\/span>\u00a0<\/span><\/p>\n
int main() \r\n{ \r\n    return GetValue(-1); \r\n}<\/pre>\n
Where did -1<\/code> come from? For this discussion, that is not <\/span>important<\/span>. Let\u2019s assume it was from\u00a0<\/span>user\u2019s<\/span>\u00a0input<\/span>.\u00a0<\/span>Anyway, t<\/span>his obviously seems wrong.\u00a0<\/span>However, I didn\u2019t get any hint from compiler on potential problem with this call, even with \/Wall<\/code>. In fact, nothing is \u201cwrong\u201d\u00a0<\/span>considering the\u00a0<\/span>types involved and how they are used. But we know this is wrong.<\/span>\u00a0Does\u00a0<\/span>GetFunction<\/span><\/code>\u00a0really protect itself from this call?<\/span>\u00a0A short answer is \u2013 No.<\/span>\u00a0<\/span><\/p>\n
Proble<\/span>ms\u00a0<\/span>are<\/span>,<\/span>\u00a0<\/span>that\u00a0<\/span>you can cast any int<\/code> value to an\u00a0<\/span>enum<\/span>\u00a0type<\/span>, and\u00a0<\/span>that\u00a0<\/span>an\u00a0<\/span>enum\u2019s<\/span>\u00a0underlying type defaults to int<\/code> \u2013 signed int<\/code>. For a signed value, if you check the upper bound<\/span>\u00a0<\/span>but not its lower bound, you end up allowing negative values.<\/span>\u00a0<\/span>In the above example, it ended up calling the dangerous\u00a0<\/span>DoNotCallMe<\/span><\/code>\u00a0function,\u00a0<\/span>that happens to be right before\u00a0<\/span>the function pointer table. In real life, this\u00a0<\/span>kind of bug\u00a0<\/span>can\u00a0<\/span>lead to\u00a0<\/span>an exploitable<\/span>\u00a0security vulnerability.<\/span>\u00a0<\/span><\/p>\n
It is less likely someone\u00a0<\/span>checks<\/span>\u00a0for the lower\u00a0<\/span>bound<\/span>\u00a0but forgets to check the upper bound. However, that can also cause the same problem<\/span>, by allowing access beyond the array bound.<\/span>\u00a0<\/span><\/p>\n
Just for fun, running the above example\u00a0<\/span>produces<\/span>\u00a0the following output<\/span>\u00a0for me<\/span>:<\/span><\/p>\n
This shouldn't be called! \r\nC:\\Temp\\Sample.exe (process 9748) exited with code -1.<\/pre>\n
EnumIndex\u202fRules<\/h2>\n
The\u202f<\/span>EnumIndex<\/span>\u00a0<\/span><\/code>extension\u202f<\/span>finds\u00a0<\/span>defects\u00a0<\/span>like the one shown above,\u00a0<\/span>and reports\u202f<\/span>them through\u00a0<\/span>the following warnings:<\/span>\u00a0<\/span><\/p>\n