From d2cefd24a1b8bd9630c3adf8adbc9585e43694e1 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Tue, 16 Dec 2025 09:39:48 -0500 Subject: [PATCH 1/2] False positive fix regarding common type check idioms. --- .../ArgumentIsSizeofOrOperation.ql | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql index fa96c006b6bc..08005f14e164 100644 --- a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql @@ -11,11 +11,46 @@ import cpp import SizeOfTypeUtils +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 + ( + // Apparently derived types, eg., functoin pointers, aren't PointerType + // according to codeql, so special casing them out here. + other.getUnspecifiedType() instanceof DerivedType + or + 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 message, Expr op where exists(string tmpMsg | ( - op instanceof BinaryOperation and tmpMsg = "binary operator" + op instanceof CandidateOperation and tmpMsg = "binary operator" or op instanceof SizeofOperator and tmpMsg = "sizeof" ) and From a412b9185cdfe81c38a0574dfbad7f3ee4342282 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Tue, 16 Dec 2025 09:49:09 -0500 Subject: [PATCH 2/2] Simplifying sizeof query output messages, and making both consistent with each other. --- .../ArgumentIsSizeofOrOperation.ql | 19 +++++++------------ .../SizeOfMisuse/SizeOfConstIntMacro.ql | 6 +++--- .../ArgumentIsSizeofOrOperation.expected | 12 ++++++------ .../SizeOfMisuse/SizeOfConstIntMacro.expected | 10 +++++----- 4 files changed, 21 insertions(+), 26 deletions(-) diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql index 08005f14e164..c75a05160aa4 100644 --- a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql @@ -46,18 +46,13 @@ class CandidateOperation extends Operation { } } -from CandidateSizeofCall sizeofExpr, string message, Expr op +from CandidateSizeofCall sizeofExpr, string inMacro, string argType, Expr op where - exists(string tmpMsg | - ( - op instanceof CandidateOperation and tmpMsg = "binary operator" - or - op instanceof SizeofOperator and tmpMsg = "sizeof" - ) and - if sizeofExpr.isInMacroExpansion() - then message = tmpMsg + "(in a macro expansion)" - else message = tmpMsg + ( + 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() diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql index 001b55d1a035..2d3c89333f51 100644 --- a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql +++ b/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() diff --git a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.expected b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.expected index a607557c00be..900685dd5a27 100644 --- a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.expected +++ b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.expected @@ -1,6 +1,6 @@ -| test.cpp:89:6:89:29 | sizeof() | $@: $@ of $@ inside sizeof. | test.cpp:89:6:89:29 | sizeof() | 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() | $@: $@ of $@ inside sizeof. | test.cpp:96:6:96:30 | sizeof() | 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() | $@: $@ of $@ inside sizeof. | test.cpp:98:6:98:35 | sizeof() | 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() | $@: $@ of $@ inside sizeof. | test.cpp:101:6:101:31 | sizeof() | 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() | $@: $@ of $@ inside sizeof. | test.cpp:120:6:120:24 | sizeof() | 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() | $@: $@ of $@ inside sizeof. | test.cpp:121:6:121:18 | sizeof() | 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() | sizeof has a binary operator argument: $@. | test.cpp:89:13:89:28 | ... / ... | ... / ... | +| test.cpp:96:6:96:30 | sizeof() | sizeof has a binary operator argument: $@. | test.cpp:96:13:96:29 | ... * ... | ... * ... | +| test.cpp:98:6:98:35 | sizeof() | sizeof has a binary operator argument: $@. | test.cpp:98:13:98:34 | ... * ... | ... * ... | +| test.cpp:101:6:101:31 | sizeof() | sizeof has a sizeof operation argument: $@. | test.cpp:101:13:101:30 | sizeof(int) | sizeof(int) | +| test.cpp:120:6:120:24 | sizeof() | sizeof has a sizeof operation argument: $@. | test.cpp:120:13:120:23 | sizeof(int) | sizeof(int) | +| test.cpp:121:6:121:18 | sizeof() | sizeof has a binary operator argument: $@. | test.cpp:121:13:121:17 | ... + ... | ... + ... | diff --git a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.expected b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.expected index 562710f3fde9..42c940886356 100644 --- a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.expected +++ b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.expected @@ -1,5 +1,5 @@ -| test.cpp:75:6:75:42 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.cpp:75:6:75:42 | sizeof() | Test01 | test.cpp:48:1:48:48 | #define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d | SOMESTRUCT_ERRNO_THAT_MATTERS | -| test.cpp:83:10:83:32 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.cpp:83:10:83:32 | sizeof() | Test01 | test.cpp:2:1:2:26 | #define BAD_MACRO_CONST 5l | BAD_MACRO_CONST | -| test.cpp:84:6:84:29 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.cpp:84:6:84:29 | sizeof() | Test01 | test.cpp:3:1:3:35 | #define BAD_MACRO_CONST2 0x80005001 | BAD_MACRO_CONST2 | -| test.cpp:92:7:92:35 | sizeof() | $@: 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() | Test01 | test.cpp:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE | -| test.cpp:116:6:116:37 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.cpp:116:6:116:37 | sizeof() | Test01 | test.cpp:32:1:32:45 | #define ACE_CONDITION_SIGNATURE2 'xt' | ACE_CONDITION_SIGNATURE2 | +| test.cpp:75:6:75:42 | sizeof() | 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() | 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() | 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() | 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() | 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 |