Merge pull request #305 from microsoft/brodes/sizeof_query_fp_fixes

Fix FPs in sizeof queries

Author: MathiasVP

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,67 @@
 import cpp
 import SizeOfTypeUtils
 
-predicate isExprAConstInteger(Expr e, MacroInvocation mi) {
+/**
+ * Holds if `type` is a `Type` that typically should not be used for `sizeof` in macros or function return values.
+ */
+predicate isTypeDangerousForSizeof(Expr e) {
   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()
+    (
+      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
   )
 }
 
 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')
+  // in these cases, the precompiler turns the string value into an integer, making it appear to be
+  // a const macro of interest, but strings should be ignored.
+  not exists(Macro m | m = mi.getMacro() | m.getBody().toString().regexpMatch("^'.{4}'$")) and
+  // Special case macros that are known to be used in buffer streams
+  // where it is common index into a buffer or allocate a buffer size based on a constant
+  // this includes known protocol constants and magic numbers
+  not (
+    // ignoring any string looking like a magic number, part of the smb2 protocol or csc protocol
+    mi.getMacroName().toLowerCase().matches(["%magic%", "%smb2%", "csc_%"]) and
+    // but only ignore if the macro does not also appear to be a size or length macro
+    not mi.getMacroName().toLowerCase().matches(["%size%", "%length%"])
   )
 }
 
-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
   )
-}
-
-/**
- * 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
-    )
+  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 |
+    (
+      // Generally ignore anything from these linux headers
+      mi.getMacro().getFile().getBaseName() in [
+          "minmax.h", "build_bug.h", "kernel.h", "bug.h", "sigcontext.h"
+        ] and
+      mi.getMacro().getFile().getRelativePath().toLowerCase().matches("%linux%")
+      or
+      // Sometimes the same macros are copied into other files, so also check the macro name
+      // this is redundant, but the first check above blocks all macros in these headers
+      // while this second check blocks any copies of these specific macros if found elsewhere.
+      mi.getMacroName() = "FP_XSTATE_MAGIC2_SIZE"
+      or
+      mi.getMacroName() = "__typecheck"
+    ) 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%")
+  or
+  // The user seems to be ignoring the output of the sizeof by casting the sizeof to void
+  // this has been observed as a common pattern in assert macros (I believe for disabling asserts in release builds).
+  // NOTE: having to check the conversion's type rather than the conversion itself
+  // i.e., checking if VoidConversion
+  // as nesting in parenthesis creats a ParenConversion
+  sizeofExpr.getExplicitlyConverted().getUnspecifiedType() instanceof VoidType
+  or
+  // A common macro seen that gets size of arguments, considered ignorable
+  exists(MacroInvocation mi |
+    mi.getMacroName() = "_SDT_ARGSIZE" and mi.getAnExpandedElement() = sizeofExpr
   )
 }
 
-/**
- * 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))
-  }
+class CandidateSizeofCall extends SizeofExprOperator {
+  CandidateSizeofCall() { not ignorableSizeof(this) }
 }