Fix FPs in sizeof queries

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) }
 }