Initial refactor to address false positives in sizeof misuse queries.

Author: bdrodes

cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql

@@ -11,39 +11,7 @@
 import cpp
 import SizeOfTypeUtils
 
-/**
- * Windows SDK corecrt_math.h defines a macro _CLASS_ARG that
- * intentionally misuses sizeof to determine the size of a floating point type.
- * Explicitly ignoring any hit in this macro.
- */
-predicate isPartOfCrtFloatingPointMacroExpansion(Expr e) {
-  exists(MacroInvocation mi |
-    mi.getMacroName() = "_CLASS_ARG" and
-    mi.getMacro().getFile().getBaseName() = "corecrt_math.h" and
-    mi.getAnExpandedElement() = e
-  )
-}
-
-/**
- * Determines if the sizeOfExpr is ignorable.
- */
-predicate ignorableSizeof(SizeofExprOperator sizeofExpr) {
-  // a common pattern found is to sizeof a binary operation to check a type
-  // to then perfomr an onperaiton for a 32 or 64 bit type.
-  // these cases often look like sizeof(x) >=4
-  // more generally we see binary operations frequently used in different type
-  // checks, where the sizeof is part of some comparison operation of a switch statement guard.
-  // sizeof as an argument is also similarly used, but seemingly less frequently.
-  exists(ComparisonOperation comp | comp.getAnOperand() = sizeofExpr)
-  or
-  exists(ConditionalStmt s | s.getControllingExpr() = sizeofExpr)
-  or
-  // another common practice is to use bit-wise operations in sizeof to allow the compiler to
-  // 'pack' the size appropriate but get the size of the result out of a sizeof operation.
-  sizeofExpr.getExprOperand() instanceof BinaryBitwiseOperation
-}
-
-from SizeofExprOperator sizeofExpr, string message, Expr op
+from CandidateSizeofCall sizeofExpr, string message, Expr op
 where
   exists(string tmpMsg |
     (
@@ -55,8 +23,6 @@ where
     then message = tmpMsg + "(in a macro expansion)"
     else message = tmpMsg
   ) and
-  op = sizeofExpr.getExprOperand() and
-  not isPartOfCrtFloatingPointMacroExpansion(op) and
-  not ignorableSizeof(sizeofExpr)
+  op = sizeofExpr.getExprOperand()
 select sizeofExpr, "$@: $@ of $@ inside sizeof.", sizeofExpr, message,
   sizeofExpr.getEnclosingFunction(), "Usage", op, message

cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql

@@ -11,44 +11,38 @@
 import cpp
 import SizeOfTypeUtils
 
-predicate isExprAConstInteger(Expr e, MacroInvocation mi) {
-  exists(Type type |
-    type = e.getExplicitlyConverted().getType() and
-    isTypeDangerousForSizeof(type) and
-    // Special case for wide-char literals when the compiler doesn't recognize wchar_t (i.e. L'\\', L'\0')
-    // Accounting for parenthesis "()" around the value
-    not exists(Macro m | m = mi.getMacro() |
-      m.getBody().toString().regexpMatch("^[\\s(]*L'.+'[\\s)]*$")
-    ) and
-    // Special case for token pasting operator
-    not exists(Macro m | m = mi.getMacro() | m.getBody().toString().regexpMatch("^.*\\s*##\\s*.*$")) and
-    // Special case for multichar literal integers that are exactly 4 character long (i.e. 'val1')
-    not exists(Macro m | m = mi.getMacro() |
-      e.getType().toString() = "int" and
-      m.getBody().toString().regexpMatch("^'.{4}'$")
-    ) and
-    e.isConstant()
-  )
-}
-
 int countMacros(Expr e) { result = count(MacroInvocation mi | mi.getExpr() = e | mi) }
 
 predicate isSizeOfExprOperandMacroInvocationAConstInteger(
-  SizeofExprOperator sizeofExpr, MacroInvocation mi
+  CandidateSizeofCall sizeofExpr, MacroInvocation mi, Literal l
 ) {
-  exists(Expr e |
-    e = mi.getExpr() and
-    e = sizeofExpr.getExprOperand() and
-    isExprAConstInteger(e, mi) and
-    // Special case for FPs that involve an inner macro that resolves to 0 such as _T('\0')
-    not exists(int macroCount | macroCount = countMacros(e) |
-      macroCount > 1 and e.(Literal).getValue().toInt() = 0
-    )
-  )
+  isTypeDangerousForSizeof(sizeofExpr.getExprOperand()) and
+  l = mi.getExpr() and
+  l = sizeofExpr.getExprOperand() and
+  mi.getExpr() = l and
+  // Special case for FPs that involve an inner macro that resolves to 0 such as _T('\0')
+  // i.e., if a macro resolves to 0, the same 0 expression cannot be the macro
+  // resolution of another macro invocation (a nested invocation).
+  // Count the number of invocations resolving to the same literal, if >1, ignore.
+  not exists(int macroCount | macroCount = countMacros(l) |
+    macroCount > 1 and l.getValue().toInt() = 0
+  ) and
+  // Special case for wide-char literals when the compiler doesn't recognize wchar_t (i.e. L'\\', L'\0')
+  // Accounting for parenthesis "()" around the value
+  not exists(Macro m | m = mi.getMacro() |
+    m.getBody().toString().regexpMatch("^[\\s(]*L'.+'[\\s)]*$")
+  ) and
+  // Special case for token pasting operator
+  not exists(Macro m | m = mi.getMacro() | m.getBody().toString().regexpMatch("^.*\\s*##\\s*.*$")) and
+  // Special case for multichar literal integers that are exactly 4 character long (i.e. 'val1')
+  not exists(Macro m | m = mi.getMacro() | m.getBody().toString().regexpMatch("^'.{4}'$"))
 }
 
-from SizeofExprOperator sizeofExpr, MacroInvocation mi
-where isSizeOfExprOperandMacroInvocationAConstInteger(sizeofExpr, mi)
+from CandidateSizeofCall sizeofExpr, MacroInvocation mi, string inMacro
+where
+  isSizeOfExprOperandMacroInvocationAConstInteger(sizeofExpr, mi, _) and
+  (if sizeofExpr.isInMacroExpansion() then inMacro = " (in a macro expansion) " else inMacro = " ")
 select sizeofExpr,
-  "$@: sizeof of integer macro $@ will always return the size of the underlying integer type.",
-  sizeofExpr, sizeofExpr.getEnclosingFunction().getName(), mi.getMacro(), mi.getMacro().getName()
+  "$@: sizeof" + inMacro +
+    "of integer macro $@ will always return the size of the underlying integer type.", sizeofExpr,
+  sizeofExpr.getEnclosingFunction().getName(), mi.getMacro(), mi.getMacro().getName()

cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfTypeUtils.qll

@@ -1,45 +1,83 @@
 import cpp
 
 /**
- * Holds if `type` is a `Type` that typically should not be used for `sizeof` in macros or function return values.
+ * Determines if the sizeOfExpr is ignorable.
  */
-predicate isTypeDangerousForSizeof(Type type) {
-  (
-    type instanceof IntegralOrEnumType and
-    // ignore string literals
-    not type instanceof WideCharType and
-    not type instanceof CharType
+predicate ignorableSizeof(SizeofExprOperator sizeofExpr) {
+  // a common pattern found is to sizeof a binary operation to check a type
+  // to then perfomr an onperaiton for a 32 or 64 bit type.
+  // these cases often look like sizeof(x) >=4
+  // more generally we see binary operations frequently used in different type
+  // checks, where the sizeof is part of some comparison operation of a switch statement guard.
+  // sizeof as an argument is also similarly used, but seemingly less frequently.
+  exists(ComparisonOperation comp | comp.getAnOperand() = sizeofExpr)
+  or
+  exists(ConditionalStmt s | s.getControllingExpr() = sizeofExpr)
+  or
+  // another common practice is to use bit-wise operations in sizeof to allow the compiler to
+  // 'pack' the size appropriate but get the size of the result out of a sizeof operation.
+  sizeofExpr.getExprOperand() instanceof BinaryBitwiseOperation
+  or
+  // Known intentional misuses in corecrt_math.h
+  // Windows SDK corecrt_math.h defines a macro _CLASS_ARG that
+  // intentionally misuses sizeof to determine the size of a floating point type.
+  // Explicitly ignoring any hit in this macro.
+  exists(MacroInvocation mi |
+    mi.getMacroName() = "_CLASS_ARG" and
+    mi.getMacro().getFile().getBaseName() = "corecrt_math.h" and
+    mi.getAnExpandedElement() = sizeofExpr
+  )
+  or
+  // the linux minmax.h header has macros that intentionally miuse sizeof,
+  // for type checking, see __typecheck
+  // This code has been observed in kernel.h as well.
+  // Ignoring cases in linux build_bug.h and bug.h see BUILD_BUG_ON_INVALID
+  // Ignoring cases of uses of FP_XSTATE_MAGIC2_SIZE found in sigcontext.h
+  // which uses sizeof a constant as a way to get an architecturally agnostic size by
+  // using the special magic number constant already defined
+  exists(MacroInvocation mi |
+    (
+      mi.getMacro().getFile().getBaseName() in [
+          "minmax.h", "build_bug.h", "kernel.h", "bug.h", "sigcontext.h"
+        ] and
+      mi.getMacro().getFile().getRelativePath().toLowerCase().matches("%linux%")
+    ) and
+    mi.getAnExpandedElement() = sizeofExpr
+  )
+  or
+  // if the operand is a macro invocation of something resembling "null"
+  // assume sizeof is intended for strings, and ignore as this is a
+  // potential null pointer issue, not a misuse of sizeof.
+  exists(MacroInvocation mi |
+    mi.getAnExpandedElement() = sizeofExpr.getExprOperand() and
+    mi.getMacroName().toLowerCase().matches("%null%")
   )
+  or
+  // LLVM has known test cases under gcc-torture, ignore any hits under any matching directory
+  // see for example 20020226-1.c
+  sizeofExpr.getFile().getRelativePath().toLowerCase().matches("%gcc-%torture%")
+}
+
+class CandidateSizeofCall extends SizeofExprOperator {
+  CandidateSizeofCall() { not ignorableSizeof(this) }
 }
 
 /**
  * Holds if `type` is a `Type` that typically should not be used for `sizeof` in macros or function return values.
- * This predicate extends the types detected in exchange of precision.
- * For higher precision, please use `isTypeDangerousForSizeof`
  */
-predicate isTypeDangerousForSizeofLowPrecision(Type type) {
-  (
-    // UINT8/BYTE are typedefs to char, so we treat them separately.
-    // WCHAR is sometimes a typedef to UINT16, so we treat it separately too.
-    type.getName() = "UINT8"
-    or
-    type.getName() = "BYTE"
-    or
-    not type.getName() = "WCHAR" and
-    exists(Type ut |
-      ut = type.getUnderlyingType() and
-      ut instanceof IntegralOrEnumType and
-      not ut instanceof WideCharType and
-      not ut instanceof CharType
+predicate isTypeDangerousForSizeof(Expr e) {
+  exists(Type type |
+    (
+      if e.getImplicitlyConverted().hasExplicitConversion()
+      then type = e.getExplicitlyConverted().getType()
+      else type = e.getUnspecifiedType()
+    )
+  |
+    (
+      type instanceof IntegralOrEnumType and
+      // ignore string literals
+      not type instanceof WideCharType and
+      not type instanceof CharType
     )
   )
 }
-
-/**
- * Holds if the `Function` return type is dangerous as input for `sizeof`.
- */
-class FunctionWithTypeDangerousForSizeofLowPrecision extends Function {
-  FunctionWithTypeDangerousForSizeofLowPrecision() {
-    exists(Type type | type = this.getType() | isTypeDangerousForSizeofLowPrecision(type))
-  }
-}

cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.expected

@@ -1,24 +1,20 @@
 | test2.c:72:6:72:42 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:72:6:72:42 | sizeof(<expr>) | Test01 | test2.c:46:1:46:48 | #define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d | SOMESTRUCT_ERRNO_THAT_MATTERS |
 | test2.c:80:10:80:32 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:80:10:80:32 | sizeof(<expr>) | Test01 | test2.c:2:1:2:26 | #define BAD_MACRO_CONST 5l | BAD_MACRO_CONST |
 | test2.c:81:6:81:29 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:81:6:81:29 | sizeof(<expr>) | Test01 | test2.c:3:1:3:35 | #define BAD_MACRO_CONST2 0x80005001 | BAD_MACRO_CONST2 |
-| test2.c:89:7:89:35 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:89:7:89:35 | sizeof(<expr>) | Test01 | test2.c:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE |
-| test2.c:98:6:98:31 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:98:6:98:31 | sizeof(<expr>) | Test01 | test2.c:17:1:17:40 | #define SOME_SIZEOF_MACRO2 (sizeof(int)) | SOME_SIZEOF_MACRO2 |
+| test2.c:89:7:89:35 | sizeof(<expr>) | $@: sizeof (in a macro expansion) of integer macro $@ will always return the size of the underlying integer type. | test2.c:89:7:89:35 | sizeof(<expr>) | Test01 | test2.c:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE |
 | test2.c:112:6:112:37 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:112:6:112:37 | sizeof(<expr>) | Test01 | test2.c:31:1:31:45 | #define ACE_CONDITION_SIGNATURE2 'xt' | ACE_CONDITION_SIGNATURE2 |
 | test2.cpp:75:6:75:42 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:75:6:75:42 | sizeof(<expr>) | Test01 | test2.cpp:48:1:48:48 | #define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d | SOMESTRUCT_ERRNO_THAT_MATTERS |
 | test2.cpp:83:10:83:32 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:83:10:83:32 | sizeof(<expr>) | Test01 | test2.cpp:2:1:2:26 | #define BAD_MACRO_CONST 5l | BAD_MACRO_CONST |
 | test2.cpp:84:6:84:29 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:84:6:84:29 | sizeof(<expr>) | Test01 | test2.cpp:3:1:3:35 | #define BAD_MACRO_CONST2 0x80005001 | BAD_MACRO_CONST2 |
-| test2.cpp:92:7:92:35 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:92:7:92:35 | sizeof(<expr>) | Test01 | test2.cpp:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE |
-| test2.cpp:101:6:101:31 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:101:6:101:31 | sizeof(<expr>) | Test01 | test2.cpp:17:1:17:40 | #define SOME_SIZEOF_MACRO2 (sizeof(int)) | SOME_SIZEOF_MACRO2 |
+| test2.cpp:92:7:92:35 | sizeof(<expr>) | $@: sizeof (in a macro expansion) of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:92:7:92:35 | sizeof(<expr>) | Test01 | test2.cpp:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE |
 | test2.cpp:116:6:116:37 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:116:6:116:37 | sizeof(<expr>) | Test01 | test2.cpp:32:1:32:45 | #define ACE_CONDITION_SIGNATURE2 'xt' | ACE_CONDITION_SIGNATURE2 |
 | test.c:72:6:72:42 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:72:6:72:42 | sizeof(<expr>) | Test01 | test.c:46:1:46:48 | #define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d | SOMESTRUCT_ERRNO_THAT_MATTERS |
 | test.c:80:10:80:32 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:80:10:80:32 | sizeof(<expr>) | Test01 | test.c:2:1:2:26 | #define BAD_MACRO_CONST 5l | BAD_MACRO_CONST |
 | test.c:81:6:81:29 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:81:6:81:29 | sizeof(<expr>) | Test01 | test.c:3:1:3:35 | #define BAD_MACRO_CONST2 0x80005001 | BAD_MACRO_CONST2 |
-| test.c:89:7:89:35 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:89:7:89:35 | sizeof(<expr>) | Test01 | test.c:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE |
-| test.c:98:6:98:31 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:98:6:98:31 | sizeof(<expr>) | Test01 | test.c:17:1:17:40 | #define SOME_SIZEOF_MACRO2 (sizeof(int)) | SOME_SIZEOF_MACRO2 |
+| test.c:89:7:89:35 | sizeof(<expr>) | $@: sizeof (in a macro expansion) of integer macro $@ will always return the size of the underlying integer type. | test.c:89:7:89:35 | sizeof(<expr>) | Test01 | test.c:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE |
 | test.c:112:6:112:37 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:112:6:112:37 | sizeof(<expr>) | Test01 | test.c:31:1:31:45 | #define ACE_CONDITION_SIGNATURE2 'xt' | ACE_CONDITION_SIGNATURE2 |
 | test.cpp:75:6:75:42 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.cpp:75:6:75:42 | sizeof(<expr>) | Test01 | test.cpp:48:1:48:48 | #define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d | SOMESTRUCT_ERRNO_THAT_MATTERS |
 | test.cpp:83:10:83:32 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.cpp:83:10:83:32 | sizeof(<expr>) | Test01 | test.cpp:2:1:2:26 | #define BAD_MACRO_CONST 5l | BAD_MACRO_CONST |
 | test.cpp:84:6:84:29 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.cpp:84:6:84:29 | sizeof(<expr>) | Test01 | test.cpp:3:1:3:35 | #define BAD_MACRO_CONST2 0x80005001 | BAD_MACRO_CONST2 |
-| test.cpp:92:7:92:35 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.cpp:92:7:92:35 | sizeof(<expr>) | Test01 | test.cpp:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE |
-| test.cpp:101:6:101:31 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.cpp:101:6:101:31 | sizeof(<expr>) | Test01 | test.cpp:17:1:17:40 | #define SOME_SIZEOF_MACRO2 (sizeof(int)) | SOME_SIZEOF_MACRO2 |
+| test.cpp:92:7:92:35 | sizeof(<expr>) | $@: sizeof (in a macro expansion) of integer macro $@ will always return the size of the underlying integer type. | test.cpp:92:7:92:35 | sizeof(<expr>) | Test01 | test.cpp:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE |
 | test.cpp:116:6:116:37 | sizeof(<expr>) | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.cpp:116:6:116:37 | sizeof(<expr>) | Test01 | test.cpp:32:1:32:45 | #define ACE_CONDITION_SIGNATURE2 'xt' | ACE_CONDITION_SIGNATURE2 |

cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.c

@@ -95,7 +95,7 @@ void Test01() {
 	x = sizeof(SOME_SIZEOF_MACRO_CAST) * 3; //BUG: ArgumentIsSizeofOrOperation
 
 	x = SOME_SIZEOF_MACRO2; // GOOD
-	x = sizeof(SOME_SIZEOF_MACRO2); //BUG: SizeOfConstIntMacro, ArgumentIsSizeofOrOperation
+	x = sizeof(SOME_SIZEOF_MACRO2); //BUG: ArgumentIsSizeofOrOperation
 
 	x = sizeof(a) / sizeof(int); // GOOD
 

cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.cpp

@@ -98,7 +98,7 @@ void Test01() {
 	x = sizeof(SOME_SIZEOF_MACRO_CAST) * 3; //BUG: ArgumentIsSizeofOrOperation
 
 	x = SOME_SIZEOF_MACRO2; // GOOD
-	x = sizeof(SOME_SIZEOF_MACRO2); //BUG: SizeOfConstIntMacro, ArgumentIsSizeofOrOperation
+	x = sizeof(SOME_SIZEOF_MACRO2); //BUG: ArgumentIsSizeofOrOperation
 
 	x = sizeof(a) / sizeof(int); // GOOD
 

cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.c

@@ -95,7 +95,7 @@ void Test01() {
 	x = sizeof(SOME_SIZEOF_MACRO_CAST) * 3; //BUG: ArgumentIsSizeofOrOperation
 
 	x = SOME_SIZEOF_MACRO2; // GOOD
-	x = sizeof(SOME_SIZEOF_MACRO2); //BUG: SizeOfConstIntMacro, ArgumentIsSizeofOrOperation
+	x = sizeof(SOME_SIZEOF_MACRO2); //BUG: ArgumentIsSizeofOrOperation
 
 	x = sizeof(a) / sizeof(int); // GOOD
 

cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.cpp

@@ -98,7 +98,7 @@ void Test01() {
 	x = sizeof(SOME_SIZEOF_MACRO_CAST) * 3; //BUG: ArgumentIsSizeofOrOperation
 
 	x = SOME_SIZEOF_MACRO2; // GOOD
-	x = sizeof(SOME_SIZEOF_MACRO2); //BUG: SizeOfConstIntMacro, ArgumentIsSizeofOrOperation
+	x = sizeof(SOME_SIZEOF_MACRO2); //BUG: ArgumentIsSizeofOrOperation
 
 	x = sizeof(a) / sizeof(int); // GOOD