Brodes/nested sizeof or operation in sizeof audit fixes (#310)

* False positive fix regarding common type check idioms.

* Simplifying sizeof query output messages, and making both consistent with each other.

Author: bdrodes

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

@@ -11,18 +11,48 @@
 import cpp
 import SizeOfTypeUtils
 
-from CandidateSizeofCall sizeofExpr, string message, Expr op
-where
-  exists(string tmpMsg |
+predicate isIgnorableBinaryOperation(BinaryOperation op) {
+  // FP case: precompilation type checking idiom of the form:
+  //    sizeof((type *)0 == (ptr))
+  op instanceof EqualityOperation and
+  exists(Literal zeroOperand, Expr other, Type t |
+    other = op.getAnOperand() and
+    other != zeroOperand and
+    zeroOperand = op.getAnOperand() and
+    zeroOperand.getValue().toInt() = 0 and
+    zeroOperand.getImplicitlyConverted().hasExplicitConversion() and
+    zeroOperand.getExplicitlyConverted().getUnspecifiedType() = t and
+    // often 'NULL' is defined as (void *)0, ignore these cases
+    not t instanceof VoidPointerType and
     (
-      op instanceof BinaryOperation and tmpMsg = "binary operator"
+      // Apparently derived types, eg., functoin pointers, aren't PointerType
+      // according to codeql, so special casing them out here.
+      other.getUnspecifiedType() instanceof DerivedType
       or
-      op instanceof SizeofOperator and tmpMsg = "sizeof"
-    ) and
-    if sizeofExpr.isInMacroExpansion()
-    then message = tmpMsg + "(in a macro expansion)"
-    else message = tmpMsg
+      other.getUnspecifiedType() instanceof PointerType
+    )
+  )
+}
+
+class CandidateOperation extends Operation {
+  CandidateOperation() {
+    // For now only considering binary operations
+    // TODO: Unary operations may be of interest but need special care
+    // as pointer deref, and address-of are unary operations.
+    // It is therefore more likely to get false positives if unary operations are included.
+    // To be considered in the future.
+    this instanceof BinaryOperation and
+    not isIgnorableBinaryOperation(this)
+  }
+}
+
+from CandidateSizeofCall sizeofExpr, string inMacro, string argType, Expr op
+where
+  (
+    op instanceof CandidateOperation and argType = "binary operator"
+    or
+    op instanceof SizeofOperator and argType = "sizeof operation"
   ) and
+  (if sizeofExpr.isInMacroExpansion() then inMacro = " (in a macro expansion) " else inMacro = " ") and
   op = sizeofExpr.getExprOperand()
-select sizeofExpr, "$@: $@ of $@ inside sizeof.", sizeofExpr, message,
-  sizeofExpr.getEnclosingFunction(), "Usage", op, message
+select sizeofExpr, "sizeof" + inMacro + "has a " + argType + " argument: $@.", op, op.toString()

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

@@ -72,6 +72,6 @@ where
   isSizeOfExprOperandMacroInvocationAConstInteger(sizeofExpr, mi, _) and
   (if sizeofExpr.isInMacroExpansion() then inMacro = " (in a macro expansion) " else inMacro = " ")
 select sizeofExpr,
-  "$@: sizeof" + inMacro +
-    "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.",
+  mi.getMacro(), mi.getMacro().getName()

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

@@ -1,6 +1,6 @@
-| test.cpp:89:6:89:29 | sizeof(<expr>) | $@: $@ of $@ inside sizeof. | test.cpp:89:6:89:29 | sizeof(<expr>) | binary operator | test.cpp:66:6:66:11 | Test01 | Usage | test.cpp:89:13:89:28 | ... / ... | binary operator |
-| test.cpp:96:6:96:30 | sizeof(<expr>) | $@: $@ of $@ inside sizeof. | test.cpp:96:6:96:30 | sizeof(<expr>) | binary operator | test.cpp:66:6:66:11 | Test01 | Usage | test.cpp:96:13:96:29 | ... * ... | binary operator |
-| test.cpp:98:6:98:35 | sizeof(<expr>) | $@: $@ of $@ inside sizeof. | test.cpp:98:6:98:35 | sizeof(<expr>) | binary operator | test.cpp:66:6:66:11 | Test01 | Usage | test.cpp:98:13:98:34 | ... * ... | binary operator |
-| test.cpp:101:6:101:31 | sizeof(<expr>) | $@: $@ of $@ inside sizeof. | test.cpp:101:6:101:31 | sizeof(<expr>) | sizeof | test.cpp:66:6:66:11 | Test01 | Usage | test.cpp:101:13:101:30 | sizeof(int) | sizeof |
-| test.cpp:120:6:120:24 | sizeof(<expr>) | $@: $@ of $@ inside sizeof. | test.cpp:120:6:120:24 | sizeof(<expr>) | sizeof | test.cpp:66:6:66:11 | Test01 | Usage | test.cpp:120:13:120:23 | sizeof(int) | sizeof |
-| test.cpp:121:6:121:18 | sizeof(<expr>) | $@: $@ of $@ inside sizeof. | test.cpp:121:6:121:18 | sizeof(<expr>) | binary operator | test.cpp:66:6:66:11 | Test01 | Usage | test.cpp:121:13:121:17 | ... + ... | binary operator |
+| test.cpp:89:6:89:29 | sizeof(<expr>) | sizeof has a binary operator argument: $@. | test.cpp:89:13:89:28 | ... / ... | ... / ... |
+| test.cpp:96:6:96:30 | sizeof(<expr>) | sizeof has a binary operator argument: $@. | test.cpp:96:13:96:29 | ... * ... | ... * ... |
+| test.cpp:98:6:98:35 | sizeof(<expr>) | sizeof has a binary operator argument: $@. | test.cpp:98:13:98:34 | ... * ... | ... * ... |
+| test.cpp:101:6:101:31 | sizeof(<expr>) | sizeof has a sizeof operation argument: $@. | test.cpp:101:13:101:30 | sizeof(int) | sizeof(int) |
+| test.cpp:120:6:120:24 | sizeof(<expr>) | sizeof has a sizeof operation argument: $@. | test.cpp:120:13:120:23 | sizeof(int) | sizeof(int) |
+| test.cpp:121:6:121:18 | sizeof(<expr>) | sizeof has a binary operator argument: $@. | test.cpp:121:13:121:17 | ... + ... | ... + ... |

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

@@ -1,5 +1,5 @@
-| 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 (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 |
+| test.cpp:75:6:75:42 | sizeof(<expr>) | sizeof of integer macro $@ will always return the size of the underlying integer type. | 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: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:3:1:3:35 | #define BAD_MACRO_CONST2 0x80005001 | BAD_MACRO_CONST2 |
+| 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: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:32:1:32:45 | #define ACE_CONDITION_SIGNATURE2 'xt' | ACE_CONDITION_SIGNATURE2 |