Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bdrodes merged 2 commits into from
Dec 16, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2cefd2
False positive fix regarding common type check idioms.
bdrodes Dec 16, 2025
a412b91
Simplifying sizeof query output messages, and making both consistent …
bdrodes Dec 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 41 additions & 11 deletions cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +28 to +32
Copy link
Collaborator

@MathiasVP MathiasVP Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you really mean for other.getUnspecifiedType() instanceof DerivedType? It's true that funmction pointers aren't PointerTypes, but then I would have assumed that you would do:

other.getUnspecifiedType() instanceof FunctionPointerType

(or maybe FunctionPointerIshType.), but you use DerivedType which includes lots of other things such as reference types, and const/volatile types (but you are calling getUnspecifiedType so const/volatiles are removed. All in all, this is very confusing 😂 Also, since PointerTypes are also DerivedTypes what you wrote is equivalent to:

other.getUnspecifiedType() instanceof DerivedType

Copy link
Author

@bdrodes bdrodes Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed they shared a common ancestor of derived. I should just make it instanceof Derived Type like you mentioned. I'll make that change. I don't mind that derived type includes other things, but I if Derived type does include specifiers for some reason... that is confusing. Basically I wanted to make the pattern as generic as possible. (TYPE*)0 == (other). I suppose I could maybe ignore "other's" type might be the most general solution.

Copy link
Author

@bdrodes bdrodes Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diving more, yea it is confusing that derived type includes the const/volatile specifiers. That's unfortunate. I'm going to special case to just function pointer as you suggest and see if it changes my results. If not, I'll submit a new PR with this fix.

)
)
}

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()
6 changes: 3 additions & 3 deletions cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
12 changes: 6 additions & 6 deletions .../test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 | ... + ... | ... + ... |
10 changes: 5 additions & 5 deletions cpp/ql/test/query-tests/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 |