From c0d29f2dc140837a5c9eef877d5526b2f5dc0c77 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Tue, 2 Dec 2025 14:26:15 -0500 Subject: [PATCH 1/4] Initial refactor to address false positives in sizeof misuse queries. --- .../ArgumentIsSizeofOrOperation.ql | 38 +------ .../SizeOfMisuse/SizeOfConstIntMacro.ql | 62 +++++------ .../SizeOfMisuse/SizeOfTypeUtils.qll | 102 ++++++++++++------ .../SizeOfMisuse/SizeOfConstIntMacro.expected | 12 +-- .../Microsoft/Likely Bugs/SizeOfMisuse/test.c | 2 +- .../Likely Bugs/SizeOfMisuse/test.cpp | 2 +- .../Likely Bugs/SizeOfMisuse/test2.c | 2 +- .../Likely Bugs/SizeOfMisuse/test2.cpp | 2 +- 8 files changed, 108 insertions(+), 114 deletions(-) diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql index 4a20a36d4f2f..fa96c006b6bc 100644 --- a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql +++ b/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 diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql index 709a33865924..e0cedfacb7b4 100644 --- a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql +++ b/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() diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfTypeUtils.qll b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfTypeUtils.qll index 87e5b1fa0f4b..d574f298d1ec 100644 --- a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfTypeUtils.qll +++ b/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)) - } -} 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 bf751cf1e9b6..caddf0d2d91d 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,24 +1,20 @@ | test2.c:72:6:72:42 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:72:6:72:42 | sizeof() | Test01 | test2.c:46:1:46:48 | #define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d | SOMESTRUCT_ERRNO_THAT_MATTERS | | test2.c:80:10:80:32 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:80:10:80:32 | sizeof() | Test01 | test2.c:2:1:2:26 | #define BAD_MACRO_CONST 5l | BAD_MACRO_CONST | | test2.c:81:6:81:29 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:81:6:81:29 | sizeof() | Test01 | test2.c:3:1:3:35 | #define BAD_MACRO_CONST2 0x80005001 | BAD_MACRO_CONST2 | -| test2.c:89:7:89:35 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:89:7:89:35 | sizeof() | Test01 | test2.c:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE | -| test2.c:98:6:98:31 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:98:6:98:31 | sizeof() | Test01 | test2.c:17:1:17:40 | #define SOME_SIZEOF_MACRO2 (sizeof(int)) | SOME_SIZEOF_MACRO2 | +| test2.c:89:7:89:35 | sizeof() | $@: 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() | Test01 | test2.c:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE | | test2.c:112:6:112:37 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:112:6:112:37 | sizeof() | Test01 | test2.c:31:1:31:45 | #define ACE_CONDITION_SIGNATURE2 'xt' | ACE_CONDITION_SIGNATURE2 | | test2.cpp:75:6:75:42 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:75:6:75:42 | sizeof() | Test01 | test2.cpp:48:1:48:48 | #define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d | SOMESTRUCT_ERRNO_THAT_MATTERS | | test2.cpp:83:10:83:32 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:83:10:83:32 | sizeof() | Test01 | test2.cpp:2:1:2:26 | #define BAD_MACRO_CONST 5l | BAD_MACRO_CONST | | test2.cpp:84:6:84:29 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:84:6:84:29 | sizeof() | Test01 | test2.cpp:3:1:3:35 | #define BAD_MACRO_CONST2 0x80005001 | BAD_MACRO_CONST2 | -| test2.cpp:92:7:92:35 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:92:7:92:35 | sizeof() | Test01 | test2.cpp:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE | -| test2.cpp:101:6:101:31 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:101:6:101:31 | sizeof() | Test01 | test2.cpp:17:1:17:40 | #define SOME_SIZEOF_MACRO2 (sizeof(int)) | SOME_SIZEOF_MACRO2 | +| test2.cpp:92:7:92:35 | sizeof() | $@: 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() | Test01 | test2.cpp:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE | | test2.cpp:116:6:116:37 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:116:6:116:37 | sizeof() | Test01 | test2.cpp:32:1:32:45 | #define ACE_CONDITION_SIGNATURE2 'xt' | ACE_CONDITION_SIGNATURE2 | | test.c:72:6:72:42 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:72:6:72:42 | sizeof() | Test01 | test.c:46:1:46:48 | #define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d | SOMESTRUCT_ERRNO_THAT_MATTERS | | test.c:80:10:80:32 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:80:10:80:32 | sizeof() | Test01 | test.c:2:1:2:26 | #define BAD_MACRO_CONST 5l | BAD_MACRO_CONST | | test.c:81:6:81:29 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:81:6:81:29 | sizeof() | Test01 | test.c:3:1:3:35 | #define BAD_MACRO_CONST2 0x80005001 | BAD_MACRO_CONST2 | -| test.c:89:7:89:35 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:89:7:89:35 | sizeof() | Test01 | test.c:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE | -| test.c:98:6:98:31 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:98:6:98:31 | sizeof() | Test01 | test.c:17:1:17:40 | #define SOME_SIZEOF_MACRO2 (sizeof(int)) | SOME_SIZEOF_MACRO2 | +| test.c:89:7:89:35 | sizeof() | $@: 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() | Test01 | test.c:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE | | test.c:112:6:112:37 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:112:6:112:37 | sizeof() | Test01 | test.c:31:1:31: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: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 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:101:6:101:31 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.cpp:101:6:101:31 | sizeof() | Test01 | test.cpp:17:1:17:40 | #define SOME_SIZEOF_MACRO2 (sizeof(int)) | SOME_SIZEOF_MACRO2 | +| 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 | diff --git a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.c b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.c index b11d0b552053..8494e2ed22b9 100644 --- a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.c +++ b/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 diff --git a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.cpp b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.cpp index d9622d3c0d94..f58c150a30c2 100644 --- a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.cpp +++ b/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 diff --git a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.c b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.c index b11d0b552053..8494e2ed22b9 100644 --- a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.c +++ b/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 diff --git a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.cpp b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.cpp index d9622d3c0d94..f58c150a30c2 100644 --- a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.cpp +++ b/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 From 91b12aecc47b88b64a2e332902971aa5b7ff7719 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Fri, 5 Dec 2025 11:35:48 -0500 Subject: [PATCH 2/4] Additional FP tweaking, removing redundant test cases, not sure why they were there, but too confusing to have repeating tests. --- .../SizeOfMisuse/SizeOfConstIntMacro.ql | 31 +++- .../SizeOfMisuse/SizeOfTypeUtils.qll | 40 +++--- .../ArgumentIsSizeofOrOperation.expected | 42 ------ .../SizeOfMisuse/SizeOfConstIntMacro.expected | 15 -- .../Microsoft/Likely Bugs/SizeOfMisuse/test.c | 118 --------------- .../Likely Bugs/SizeOfMisuse/test.cpp | 9 +- .../Likely Bugs/SizeOfMisuse/test2.c | 118 --------------- .../Likely Bugs/SizeOfMisuse/test2.cpp | 136 ------------------ 8 files changed, 57 insertions(+), 452 deletions(-) delete mode 100644 cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.c delete mode 100644 cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.c delete mode 100644 cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.cpp diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql index e0cedfacb7b4..14c6ea1350e9 100644 --- a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql @@ -11,6 +11,26 @@ import cpp import SizeOfTypeUtils +/** + * 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 | + ( + 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( @@ -35,7 +55,16 @@ predicate isSizeOfExprOperandMacroInvocationAConstInteger( // 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}'$")) + 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 CandidateSizeofCall sizeofExpr, MacroInvocation mi, string inMacro diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfTypeUtils.qll b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfTypeUtils.qll index d574f298d1ec..a39439d58fd8 100644 --- a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfTypeUtils.qll +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfTypeUtils.qll @@ -37,10 +37,18 @@ predicate ignorableSizeof(SizeofExprOperator sizeofExpr) { // 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 ) @@ -56,28 +64,20 @@ predicate ignorableSizeof(SizeofExprOperator sizeofExpr) { // 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 + ) } 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. - */ -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 - ) - ) -} 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 2099532f8f4a..a607557c00be 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,48 +1,6 @@ -| test2.c:86:6:86:29 | sizeof() | $@: $@ of $@ inside sizeof. | test2.c:86:6:86:29 | sizeof() | binary operator | test2.c:64:6:64:11 | Test01 | Usage | test2.c:86:13:86:28 | ... / ... | binary operator | -| test2.c:86:6:86:29 | sizeof() | $@: $@ of $@ inside sizeof. | test2.c:86:6:86:29 | sizeof() | binary operator | test.c:64:6:64:11 | Test01 | Usage | test2.c:86:13:86:28 | ... / ... | binary operator | -| test2.c:93:6:93:30 | sizeof() | $@: $@ of $@ inside sizeof. | test2.c:93:6:93:30 | sizeof() | binary operator | test2.c:64:6:64:11 | Test01 | Usage | test2.c:93:13:93:29 | ... * ... | binary operator | -| test2.c:93:6:93:30 | sizeof() | $@: $@ of $@ inside sizeof. | test2.c:93:6:93:30 | sizeof() | binary operator | test.c:64:6:64:11 | Test01 | Usage | test2.c:93:13:93:29 | ... * ... | binary operator | -| test2.c:95:6:95:35 | sizeof() | $@: $@ of $@ inside sizeof. | test2.c:95:6:95:35 | sizeof() | binary operator | test2.c:64:6:64:11 | Test01 | Usage | test2.c:95:13:95:34 | ... * ... | binary operator | -| test2.c:95:6:95:35 | sizeof() | $@: $@ of $@ inside sizeof. | test2.c:95:6:95:35 | sizeof() | binary operator | test.c:64:6:64:11 | Test01 | Usage | test2.c:95:13:95:34 | ... * ... | binary operator | -| test2.c:98:6:98:31 | sizeof() | $@: $@ of $@ inside sizeof. | test2.c:98:6:98:31 | sizeof() | sizeof | test2.c:64:6:64:11 | Test01 | Usage | test2.c:98:13:98:30 | sizeof(int) | sizeof | -| test2.c:98:6:98:31 | sizeof() | $@: $@ of $@ inside sizeof. | test2.c:98:6:98:31 | sizeof() | sizeof | test.c:64:6:64:11 | Test01 | Usage | test2.c:98:13:98:30 | sizeof(int) | sizeof | -| test2.c:116:6:116:24 | sizeof() | $@: $@ of $@ inside sizeof. | test2.c:116:6:116:24 | sizeof() | sizeof | test2.c:64:6:64:11 | Test01 | Usage | test2.c:116:13:116:23 | sizeof(int) | sizeof | -| test2.c:116:6:116:24 | sizeof() | $@: $@ of $@ inside sizeof. | test2.c:116:6:116:24 | sizeof() | sizeof | test.c:64:6:64:11 | Test01 | Usage | test2.c:116:13:116:23 | sizeof(int) | sizeof | -| test2.c:117:6:117:18 | sizeof() | $@: $@ of $@ inside sizeof. | test2.c:117:6:117:18 | sizeof() | binary operator | test2.c:64:6:64:11 | Test01 | Usage | test2.c:117:13:117:17 | ... + ... | binary operator | -| test2.c:117:6:117:18 | sizeof() | $@: $@ of $@ inside sizeof. | test2.c:117:6:117:18 | sizeof() | binary operator | test.c:64:6:64:11 | Test01 | Usage | test2.c:117:13:117:17 | ... + ... | binary operator | -| test2.cpp:89:6:89:29 | sizeof() | $@: $@ of $@ inside sizeof. | test2.cpp:89:6:89:29 | sizeof() | binary operator | test2.cpp:66:6:66:11 | Test01 | Usage | test2.cpp:89:13:89:28 | ... / ... | binary operator | -| test2.cpp:89:6:89:29 | sizeof() | $@: $@ of $@ inside sizeof. | test2.cpp:89:6:89:29 | sizeof() | binary operator | test.cpp:66:6:66:11 | Test01 | Usage | test2.cpp:89:13:89:28 | ... / ... | binary operator | -| test2.cpp:96:6:96:30 | sizeof() | $@: $@ of $@ inside sizeof. | test2.cpp:96:6:96:30 | sizeof() | binary operator | test2.cpp:66:6:66:11 | Test01 | Usage | test2.cpp:96:13:96:29 | ... * ... | binary operator | -| test2.cpp:96:6:96:30 | sizeof() | $@: $@ of $@ inside sizeof. | test2.cpp:96:6:96:30 | sizeof() | binary operator | test.cpp:66:6:66:11 | Test01 | Usage | test2.cpp:96:13:96:29 | ... * ... | binary operator | -| test2.cpp:98:6:98:35 | sizeof() | $@: $@ of $@ inside sizeof. | test2.cpp:98:6:98:35 | sizeof() | binary operator | test2.cpp:66:6:66:11 | Test01 | Usage | test2.cpp:98:13:98:34 | ... * ... | binary operator | -| test2.cpp:98:6:98:35 | sizeof() | $@: $@ of $@ inside sizeof. | test2.cpp:98:6:98:35 | sizeof() | binary operator | test.cpp:66:6:66:11 | Test01 | Usage | test2.cpp:98:13:98:34 | ... * ... | binary operator | -| test2.cpp:101:6:101:31 | sizeof() | $@: $@ of $@ inside sizeof. | test2.cpp:101:6:101:31 | sizeof() | sizeof | test2.cpp:66:6:66:11 | Test01 | Usage | test2.cpp:101:13:101:30 | sizeof(int) | sizeof | -| test2.cpp:101:6:101:31 | sizeof() | $@: $@ of $@ inside sizeof. | test2.cpp:101:6:101:31 | sizeof() | sizeof | test.cpp:66:6:66:11 | Test01 | Usage | test2.cpp:101:13:101:30 | sizeof(int) | sizeof | -| test2.cpp:120:6:120:24 | sizeof() | $@: $@ of $@ inside sizeof. | test2.cpp:120:6:120:24 | sizeof() | sizeof | test2.cpp:66:6:66:11 | Test01 | Usage | test2.cpp:120:13:120:23 | sizeof(int) | sizeof | -| test2.cpp:120:6:120:24 | sizeof() | $@: $@ of $@ inside sizeof. | test2.cpp:120:6:120:24 | sizeof() | sizeof | test.cpp:66:6:66:11 | Test01 | Usage | test2.cpp:120:13:120:23 | sizeof(int) | sizeof | -| test2.cpp:121:6:121:18 | sizeof() | $@: $@ of $@ inside sizeof. | test2.cpp:121:6:121:18 | sizeof() | binary operator | test2.cpp:66:6:66:11 | Test01 | Usage | test2.cpp:121:13:121:17 | ... + ... | binary operator | -| test2.cpp:121:6:121:18 | sizeof() | $@: $@ of $@ inside sizeof. | test2.cpp:121:6:121:18 | sizeof() | binary operator | test.cpp:66:6:66:11 | Test01 | Usage | test2.cpp:121:13:121:17 | ... + ... | binary operator | -| test.c:86:6:86:29 | sizeof() | $@: $@ of $@ inside sizeof. | test.c:86:6:86:29 | sizeof() | binary operator | test2.c:64:6:64:11 | Test01 | Usage | test.c:86:13:86:28 | ... / ... | binary operator | -| test.c:86:6:86:29 | sizeof() | $@: $@ of $@ inside sizeof. | test.c:86:6:86:29 | sizeof() | binary operator | test.c:64:6:64:11 | Test01 | Usage | test.c:86:13:86:28 | ... / ... | binary operator | -| test.c:93:6:93:30 | sizeof() | $@: $@ of $@ inside sizeof. | test.c:93:6:93:30 | sizeof() | binary operator | test2.c:64:6:64:11 | Test01 | Usage | test.c:93:13:93:29 | ... * ... | binary operator | -| test.c:93:6:93:30 | sizeof() | $@: $@ of $@ inside sizeof. | test.c:93:6:93:30 | sizeof() | binary operator | test.c:64:6:64:11 | Test01 | Usage | test.c:93:13:93:29 | ... * ... | binary operator | -| test.c:95:6:95:35 | sizeof() | $@: $@ of $@ inside sizeof. | test.c:95:6:95:35 | sizeof() | binary operator | test2.c:64:6:64:11 | Test01 | Usage | test.c:95:13:95:34 | ... * ... | binary operator | -| test.c:95:6:95:35 | sizeof() | $@: $@ of $@ inside sizeof. | test.c:95:6:95:35 | sizeof() | binary operator | test.c:64:6:64:11 | Test01 | Usage | test.c:95:13:95:34 | ... * ... | binary operator | -| test.c:98:6:98:31 | sizeof() | $@: $@ of $@ inside sizeof. | test.c:98:6:98:31 | sizeof() | sizeof | test2.c:64:6:64:11 | Test01 | Usage | test.c:98:13:98:30 | sizeof(int) | sizeof | -| test.c:98:6:98:31 | sizeof() | $@: $@ of $@ inside sizeof. | test.c:98:6:98:31 | sizeof() | sizeof | test.c:64:6:64:11 | Test01 | Usage | test.c:98:13:98:30 | sizeof(int) | sizeof | -| test.c:116:6:116:24 | sizeof() | $@: $@ of $@ inside sizeof. | test.c:116:6:116:24 | sizeof() | sizeof | test2.c:64:6:64:11 | Test01 | Usage | test.c:116:13:116:23 | sizeof(int) | sizeof | -| test.c:116:6:116:24 | sizeof() | $@: $@ of $@ inside sizeof. | test.c:116:6:116:24 | sizeof() | sizeof | test.c:64:6:64:11 | Test01 | Usage | test.c:116:13:116:23 | sizeof(int) | sizeof | -| test.c:117:6:117:18 | sizeof() | $@: $@ of $@ inside sizeof. | test.c:117:6:117:18 | sizeof() | binary operator | test2.c:64:6:64:11 | Test01 | Usage | test.c:117:13:117:17 | ... + ... | binary operator | -| test.c:117:6:117:18 | sizeof() | $@: $@ of $@ inside sizeof. | test.c:117:6:117:18 | sizeof() | binary operator | test.c:64:6:64:11 | Test01 | Usage | test.c:117:13:117:17 | ... + ... | binary operator | -| test.cpp:89:6:89:29 | sizeof() | $@: $@ of $@ inside sizeof. | test.cpp:89:6:89:29 | sizeof() | binary operator | test2.cpp:66:6:66:11 | Test01 | Usage | test.cpp:89:13:89:28 | ... / ... | binary operator | | 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 | test2.cpp:66:6:66:11 | Test01 | Usage | test.cpp:96:13:96:29 | ... * ... | 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 | test2.cpp:66:6:66:11 | Test01 | Usage | test.cpp:98:13:98:34 | ... * ... | 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 | test2.cpp:66:6:66:11 | Test01 | Usage | test.cpp:101:13:101:30 | sizeof(int) | sizeof | | 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 | test2.cpp:66:6:66:11 | Test01 | Usage | test.cpp:120:13:120:23 | 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 | test2.cpp:66:6:66:11 | Test01 | Usage | test.cpp:121:13:121:17 | ... + ... | binary operator | | 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 | 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 caddf0d2d91d..562710f3fde9 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,18 +1,3 @@ -| test2.c:72:6:72:42 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:72:6:72:42 | sizeof() | Test01 | test2.c:46:1:46:48 | #define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d | SOMESTRUCT_ERRNO_THAT_MATTERS | -| test2.c:80:10:80:32 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:80:10:80:32 | sizeof() | Test01 | test2.c:2:1:2:26 | #define BAD_MACRO_CONST 5l | BAD_MACRO_CONST | -| test2.c:81:6:81:29 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:81:6:81:29 | sizeof() | Test01 | test2.c:3:1:3:35 | #define BAD_MACRO_CONST2 0x80005001 | BAD_MACRO_CONST2 | -| test2.c:89:7:89:35 | sizeof() | $@: 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() | Test01 | test2.c:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE | -| test2.c:112:6:112:37 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.c:112:6:112:37 | sizeof() | Test01 | test2.c:31:1:31:45 | #define ACE_CONDITION_SIGNATURE2 'xt' | ACE_CONDITION_SIGNATURE2 | -| test2.cpp:75:6:75:42 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:75:6:75:42 | sizeof() | Test01 | test2.cpp:48:1:48:48 | #define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d | SOMESTRUCT_ERRNO_THAT_MATTERS | -| test2.cpp:83:10:83:32 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:83:10:83:32 | sizeof() | Test01 | test2.cpp:2:1:2:26 | #define BAD_MACRO_CONST 5l | BAD_MACRO_CONST | -| test2.cpp:84:6:84:29 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:84:6:84:29 | sizeof() | Test01 | test2.cpp:3:1:3:35 | #define BAD_MACRO_CONST2 0x80005001 | BAD_MACRO_CONST2 | -| test2.cpp:92:7:92:35 | sizeof() | $@: 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() | Test01 | test2.cpp:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE | -| test2.cpp:116:6:116:37 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test2.cpp:116:6:116:37 | sizeof() | Test01 | test2.cpp:32:1:32:45 | #define ACE_CONDITION_SIGNATURE2 'xt' | ACE_CONDITION_SIGNATURE2 | -| test.c:72:6:72:42 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:72:6:72:42 | sizeof() | Test01 | test.c:46:1:46:48 | #define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d | SOMESTRUCT_ERRNO_THAT_MATTERS | -| test.c:80:10:80:32 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:80:10:80:32 | sizeof() | Test01 | test.c:2:1:2:26 | #define BAD_MACRO_CONST 5l | BAD_MACRO_CONST | -| test.c:81:6:81:29 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:81:6:81:29 | sizeof() | Test01 | test.c:3:1:3:35 | #define BAD_MACRO_CONST2 0x80005001 | BAD_MACRO_CONST2 | -| test.c:89:7:89:35 | sizeof() | $@: 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() | Test01 | test.c:1:1:1:19 | #define PAGESIZE 64 | PAGESIZE | -| test.c:112:6:112:37 | sizeof() | $@: sizeof of integer macro $@ will always return the size of the underlying integer type. | test.c:112:6:112:37 | sizeof() | Test01 | test.c:31:1:31: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: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 | diff --git a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.c b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.c deleted file mode 100644 index 8494e2ed22b9..000000000000 --- a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.c +++ /dev/null @@ -1,118 +0,0 @@ -#define PAGESIZE 64 -#define BAD_MACRO_CONST 5l -#define BAD_MACRO_CONST2 0x80005001 -#define BAD_MACRO_OP1(VAR) strlen(#VAR) -#define BAD_MACRO_OP2(VAR) sizeof(VAR)/sizeof(int) - -long strlen(const char* x) { return 5; } - -#define ALIGN_UP_BY(length, sizeofType) length * sizeofType -#define ALIGN_UP(length, type) \ - ALIGN_UP_BY(length, sizeof(type)) - -#define SOME_SIZEOF_MACRO (sizeof(int) * 3) -#define SOME_SIZEOF_MACRO_CAST ((char)(sizeof(int) * 3)) - - -#define SOME_SIZEOF_MACRO2 (sizeof(int)) - -typedef unsigned short WCHAR; // wc, 16-bit UNICODE character - -#define UNICODE_NULL1 ((WCHAR)0) - -#define ASCII_NULL ((char)0) - -#define UNICODE_STRING_SIG L"xtra" -#define ASCII_STRING_SIG "xtra" - -#define UNICODE_SIG L'x' - -#define ACE_CONDITION_SIGNATURE1 'xtra' -#define ACE_CONDITION_SIGNATURE2 'xt' - -#define ACE_CONDITION_SIGNATURE3(VAL) #VAL - -#define NULL (void *)0 - -#define EFI_FILEPATH_SEPARATOR_UNICODE L'\\' - -const char* Test() -{ - return "foobar"; -} - -#define FUNCTION_MACRO_OP1 Test() - -#define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d - - -char _RTL_CONSTANT_STRING_type_check(const void* s) { - return ((char*)(s))[0]; -} - -#define RTL_CONSTANT_STRING(s) \ -{ \ - sizeof( s ) - sizeof( (s)[0] ); \ - sizeof( s ) / sizeof(_RTL_CONSTANT_STRING_type_check(s)); \ -} - -typedef struct { - int a; - char b; -} SOMESTRUCT_THAT_MATTERS; - -void Test01() { - - RTL_CONSTANT_STRING("hello"); - - sizeof(NULL); - sizeof(EFI_FILEPATH_SEPARATOR_UNICODE); - - int y = sizeof(SOMESTRUCT_THAT_MATTERS); - y = sizeof(SOMESTRUCT_ERRNO_THAT_MATTERS); // BUG: SizeOfConstIntMacro - - const unsigned short* p = UNICODE_STRING_SIG; - const char* p2 = ASCII_STRING_SIG; - char p3 = 'xtra'; - unsigned short p4 = L'xtra'; - - int a[10]; - int x = sizeof(BAD_MACRO_CONST); //BUG: SizeOfConstIntMacro - x = sizeof(BAD_MACRO_CONST2); //BUG: SizeOfConstIntMacro - - x = sizeof(FUNCTION_MACRO_OP1); // GOOD - - x = sizeof(BAD_MACRO_OP1(a)); //BUG: ArgumentIsFunctionCall (Low Prec) - x = sizeof(BAD_MACRO_OP2(a)); //BUG: ArgumentIsSizeofOrOperation - - x = 0; - x += ALIGN_UP(sizeof(a), PAGESIZE); //BUG: SizeOfConstIntMacro - x += ALIGN_UP_BY(sizeof(a), PAGESIZE); // GOOD - - x = SOME_SIZEOF_MACRO * 3; // GOOD - x = sizeof(SOME_SIZEOF_MACRO) * 3; //BUG: ArgumentIsSizeofOrOperation - - x = sizeof(SOME_SIZEOF_MACRO_CAST) * 3; //BUG: ArgumentIsSizeofOrOperation - - x = SOME_SIZEOF_MACRO2; // GOOD - x = sizeof(SOME_SIZEOF_MACRO2); //BUG: ArgumentIsSizeofOrOperation - - x = sizeof(a) / sizeof(int); // GOOD - - x = sizeof(UNICODE_NULL1); - - x = sizeof(ASCII_NULL); - - x = sizeof(UNICODE_STRING_SIG); - x = sizeof(ASCII_STRING_SIG); - - x = sizeof(UNICODE_SIG); - - x = sizeof(ACE_CONDITION_SIGNATURE1); // GOOD (special case) - x = sizeof(ACE_CONDITION_SIGNATURE2); // BUG: SizeOfConstIntMacro - - x = sizeof(ACE_CONDITION_SIGNATURE3(xtra)); - - x = sizeof(sizeof(int)); // BUG: ArgumentIsSizeofOrOperation - x = sizeof(3 + 2); // BUg: ArgumentIsSizeofOrOperation -} diff --git a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.cpp b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.cpp index f58c150a30c2..86f12ba5b957 100644 --- a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.cpp +++ b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test.cpp @@ -128,9 +128,14 @@ void Test01() { #define _T(x) L ## x +#define SAFE_ASSERT(e) (((void)sizeof(e))) + void test02_FalsePositives() { int x = WNULL_SIZE; x = sizeof(RKF_PATH_UTIL_STREAM_MARKER); - sizeof(_T('\0')); -} \ No newline at end of file + sizeof(_T('\0')); // GOOD: ignorable sizeof use + + SAFE_ASSERT(sizeof(int) == 4); // GOOD: ignorable sizeof use + SAFE_ASSERT(BAD_MACRO_CONST); // GOOD: ignorable sizeof use +} diff --git a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.c b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.c deleted file mode 100644 index 8494e2ed22b9..000000000000 --- a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.c +++ /dev/null @@ -1,118 +0,0 @@ -#define PAGESIZE 64 -#define BAD_MACRO_CONST 5l -#define BAD_MACRO_CONST2 0x80005001 -#define BAD_MACRO_OP1(VAR) strlen(#VAR) -#define BAD_MACRO_OP2(VAR) sizeof(VAR)/sizeof(int) - -long strlen(const char* x) { return 5; } - -#define ALIGN_UP_BY(length, sizeofType) length * sizeofType -#define ALIGN_UP(length, type) \ - ALIGN_UP_BY(length, sizeof(type)) - -#define SOME_SIZEOF_MACRO (sizeof(int) * 3) -#define SOME_SIZEOF_MACRO_CAST ((char)(sizeof(int) * 3)) - - -#define SOME_SIZEOF_MACRO2 (sizeof(int)) - -typedef unsigned short WCHAR; // wc, 16-bit UNICODE character - -#define UNICODE_NULL1 ((WCHAR)0) - -#define ASCII_NULL ((char)0) - -#define UNICODE_STRING_SIG L"xtra" -#define ASCII_STRING_SIG "xtra" - -#define UNICODE_SIG L'x' - -#define ACE_CONDITION_SIGNATURE1 'xtra' -#define ACE_CONDITION_SIGNATURE2 'xt' - -#define ACE_CONDITION_SIGNATURE3(VAL) #VAL - -#define NULL (void *)0 - -#define EFI_FILEPATH_SEPARATOR_UNICODE L'\\' - -const char* Test() -{ - return "foobar"; -} - -#define FUNCTION_MACRO_OP1 Test() - -#define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d - - -char _RTL_CONSTANT_STRING_type_check(const void* s) { - return ((char*)(s))[0]; -} - -#define RTL_CONSTANT_STRING(s) \ -{ \ - sizeof( s ) - sizeof( (s)[0] ); \ - sizeof( s ) / sizeof(_RTL_CONSTANT_STRING_type_check(s)); \ -} - -typedef struct { - int a; - char b; -} SOMESTRUCT_THAT_MATTERS; - -void Test01() { - - RTL_CONSTANT_STRING("hello"); - - sizeof(NULL); - sizeof(EFI_FILEPATH_SEPARATOR_UNICODE); - - int y = sizeof(SOMESTRUCT_THAT_MATTERS); - y = sizeof(SOMESTRUCT_ERRNO_THAT_MATTERS); // BUG: SizeOfConstIntMacro - - const unsigned short* p = UNICODE_STRING_SIG; - const char* p2 = ASCII_STRING_SIG; - char p3 = 'xtra'; - unsigned short p4 = L'xtra'; - - int a[10]; - int x = sizeof(BAD_MACRO_CONST); //BUG: SizeOfConstIntMacro - x = sizeof(BAD_MACRO_CONST2); //BUG: SizeOfConstIntMacro - - x = sizeof(FUNCTION_MACRO_OP1); // GOOD - - x = sizeof(BAD_MACRO_OP1(a)); //BUG: ArgumentIsFunctionCall (Low Prec) - x = sizeof(BAD_MACRO_OP2(a)); //BUG: ArgumentIsSizeofOrOperation - - x = 0; - x += ALIGN_UP(sizeof(a), PAGESIZE); //BUG: SizeOfConstIntMacro - x += ALIGN_UP_BY(sizeof(a), PAGESIZE); // GOOD - - x = SOME_SIZEOF_MACRO * 3; // GOOD - x = sizeof(SOME_SIZEOF_MACRO) * 3; //BUG: ArgumentIsSizeofOrOperation - - x = sizeof(SOME_SIZEOF_MACRO_CAST) * 3; //BUG: ArgumentIsSizeofOrOperation - - x = SOME_SIZEOF_MACRO2; // GOOD - x = sizeof(SOME_SIZEOF_MACRO2); //BUG: ArgumentIsSizeofOrOperation - - x = sizeof(a) / sizeof(int); // GOOD - - x = sizeof(UNICODE_NULL1); - - x = sizeof(ASCII_NULL); - - x = sizeof(UNICODE_STRING_SIG); - x = sizeof(ASCII_STRING_SIG); - - x = sizeof(UNICODE_SIG); - - x = sizeof(ACE_CONDITION_SIGNATURE1); // GOOD (special case) - x = sizeof(ACE_CONDITION_SIGNATURE2); // BUG: SizeOfConstIntMacro - - x = sizeof(ACE_CONDITION_SIGNATURE3(xtra)); - - x = sizeof(sizeof(int)); // BUG: ArgumentIsSizeofOrOperation - x = sizeof(3 + 2); // BUg: ArgumentIsSizeofOrOperation -} diff --git a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.cpp b/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.cpp deleted file mode 100644 index f58c150a30c2..000000000000 --- a/cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/test2.cpp +++ /dev/null @@ -1,136 +0,0 @@ -#define PAGESIZE 64 -#define BAD_MACRO_CONST 5l -#define BAD_MACRO_CONST2 0x80005001 -#define BAD_MACRO_OP1(VAR) strlen(#VAR) -#define BAD_MACRO_OP2(VAR) sizeof(VAR)/sizeof(int) - -long strlen(const char* x) { return 5; } - -#define ALIGN_UP_BY(length, sizeofType) length * sizeofType -#define ALIGN_UP(length, type) \ - ALIGN_UP_BY(length, sizeof(type)) - -#define SOME_SIZEOF_MACRO (sizeof(int) * 3) -#define SOME_SIZEOF_MACRO_CAST ((char)(sizeof(int) * 3)) - - -#define SOME_SIZEOF_MACRO2 (sizeof(int)) - -typedef wchar_t WCHAR; // wc, 16-bit UNICODE character - -#define UNICODE_NULL1 ((WCHAR)0) -#define UNICODE_NULL2 ((wchar_t)0) -#define ASCII_NULL ((char)0) - -#define UNICODE_STRING_SIG L"xtra" -#define ASCII_STRING_SIG "xtra" - -#define ASCII_SIG 'x' -#define UNICODE_SIG L'x' - -#define ACE_CONDITION_SIGNATURE1 'xtra' -#define ACE_CONDITION_SIGNATURE2 'xt' - -#define ACE_CONDITION_SIGNATURE3(VAL) #VAL - -#define NULL (void *)0 - -#define EFI_FILEPATH_SEPARATOR_UNICODE L'\\' -#define EFI_FILEPATH_SEPARATOR_ASCII '\\' - -const char* Test() -{ - return "foobar"; -} - -#define FUNCTION_MACRO_OP1 Test() - -#define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d - - -char _RTL_CONSTANT_STRING_type_check(const void* s) { - return ((char*)(s))[0]; -} - -#define RTL_CONSTANT_STRING(s) \ -{ \ - sizeof( s ) - sizeof( (s)[0] ); \ - sizeof( s ) / sizeof(_RTL_CONSTANT_STRING_type_check(s)); \ -} - -typedef struct { - int a; - bool b; -} SOMESTRUCT_THAT_MATTERS; - -void Test01() { - - RTL_CONSTANT_STRING("hello"); - - sizeof(NULL); - sizeof(EFI_FILEPATH_SEPARATOR_UNICODE); - sizeof(EFI_FILEPATH_SEPARATOR_ASCII); - - int y = sizeof(SOMESTRUCT_THAT_MATTERS); - y = sizeof(SOMESTRUCT_ERRNO_THAT_MATTERS); // BUG: SizeOfConstIntMacro - - const wchar_t* p = UNICODE_STRING_SIG; - const char* p2 = ASCII_STRING_SIG; - char p3 = 'xtra'; - wchar_t p4 = L'xtra'; - - int a[10]; - int x = sizeof(BAD_MACRO_CONST); //BUG: SizeOfConstIntMacro - x = sizeof(BAD_MACRO_CONST2); //BUG: SizeOfConstIntMacro - - x = sizeof(FUNCTION_MACRO_OP1); // GOOD - - x = sizeof(BAD_MACRO_OP1(a)); //BUG: ArgumentIsFunctionCall (Low Prec) - x = sizeof(BAD_MACRO_OP2(a)); //BUG: ArgumentIsSizeofOrOperation - - x = 0; - x += ALIGN_UP(sizeof(a), PAGESIZE); //BUG: SizeOfConstIntMacro - x += ALIGN_UP_BY(sizeof(a), PAGESIZE); // GOOD - - x = SOME_SIZEOF_MACRO * 3; // GOOD - x = sizeof(SOME_SIZEOF_MACRO) * 3; //BUG: ArgumentIsSizeofOrOperation - - x = sizeof(SOME_SIZEOF_MACRO_CAST) * 3; //BUG: ArgumentIsSizeofOrOperation - - x = SOME_SIZEOF_MACRO2; // GOOD - x = sizeof(SOME_SIZEOF_MACRO2); //BUG: ArgumentIsSizeofOrOperation - - x = sizeof(a) / sizeof(int); // GOOD - - x = sizeof(UNICODE_NULL1); - x = sizeof(UNICODE_NULL2); - x = sizeof(ASCII_NULL); - - x = sizeof(UNICODE_STRING_SIG); - x = sizeof(ASCII_STRING_SIG); - - x = sizeof(ASCII_SIG); - x = sizeof(UNICODE_SIG); - - x = sizeof(ACE_CONDITION_SIGNATURE1); // GOOD (special case) - x = sizeof(ACE_CONDITION_SIGNATURE2); // BUG: SizeOfConstIntMacro - - x = sizeof(ACE_CONDITION_SIGNATURE3(xtra)); - - x = sizeof(sizeof(int)); // BUG: ArgumentIsSizeofOrOperation - x = sizeof(3 + 2); // BUg: ArgumentIsSizeofOrOperation -} - -#define WNULL (L'\0') -#define WNULL_SIZE (sizeof(WNULL)) - -#define RKF_PATH_UTIL_STREAM_MARKER ( L':' ) - -#define _T(x) L ## x - -void test02_FalsePositives() -{ - int x = WNULL_SIZE; - x = sizeof(RKF_PATH_UTIL_STREAM_MARKER); - sizeof(_T('\0')); -} \ No newline at end of file From 52628248d179898684226e0d753771d22253f4e8 Mon Sep 17 00:00:00 2001 From: Ben Rodes Date: Fri, 5 Dec 2025 12:27:07 -0500 Subject: [PATCH 3/4] Update cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql Co-authored-by: Mathias Vorreiter Pedersen --- .../Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql index 14c6ea1350e9..06cff5beb2e7 100644 --- a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql @@ -22,12 +22,10 @@ predicate isTypeDangerousForSizeof(Expr e) { else type = e.getUnspecifiedType() ) | - ( - type instanceof IntegralOrEnumType and - // ignore string literals - not type instanceof WideCharType and - not type instanceof CharType - ) + type instanceof IntegralOrEnumType and + // ignore string literals + not type instanceof WideCharType and + not type instanceof CharType ) } From c6b48c36d7c712af28ad06421c46c121a398a81e Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Fri, 5 Dec 2025 12:31:39 -0500 Subject: [PATCH 4/4] updated comment. --- .../Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql index 06cff5beb2e7..001b55d1a035 100644 --- a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql @@ -53,6 +53,8 @@ predicate isSizeOfExprOperandMacroInvocationAConstInteger( // 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