From 978a110337f7c9802cebca65a57e92bc8672b79f Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Wed, 5 Aug 2026 22:21:36 -0400 Subject: [PATCH] CodeQL: Flag implicit compare-then-assign in branch conditions Implicit compare-then-assign in branch conditions is buggy since developers often mean assign-then-compare, but sometimes actually mean compare-then-assign. GCC's -Wparentheses was originally meant to catch assignment in place of comparison, requiring an extra set of parentheses to turn this off. This had the happy coincidence of making developers explicit about assign-then-compare vs compare-then-assign. An outer level of extra parentheses will inhibit -Wparentheses warnings. This often results in assign-then-compare being made explicit, but instead of turning `if (x = foo() < 0)` into `if ((x = foo()) < 0)`, a developer might write `if ((x = foo() < 0))`, which turns off the warning, without fixing the problem. This happened in openzfs/zfs#18874. There are other potential variations, such as `if ((x = (foo()) < 0))`, which also suppresses GCC's warning, but fails to actually do anything since the intended explicit parentheses to specify compare-then-assign are around the right operand of the boolean operator, rather than around the boolean operator, yet we have the additional parentheses needed to silence GCC's -Wparentheses. In the `if ((x = (foo()) < 0))` case, the intent was to make compare-then-assign explicit, and a typo caused it to fail to become explicit. That is not a bug, but it makes it unclear what the developer intended, which is problematic in itself. This probably merits a bug report to GCC requesting a more intelligent diagnostic that will treat compare-then-assign differently from assignment in a branch condition. However, that is a slow process, this has already bitten us once and with CodeQL, we can add our own check to the PR process so that we catch other instances of this issue during review, rather than some time later. Given that assign-then-compare in branch conditions requires that parentheses be added in such a way that the compiler AST no longer contains an implicit compare-then-assign, we only need to check for an implicit compare-then-assign in order to implement this check. Although the likelihood of compound assignment being present in this bug pattern is low, the same logic follows, so the check also will catch this pattern on compound assignment. This check handles conditions in if, while, do, for, ?:, && and ||. switch statements are explicitly ignored, since using assign-then-compare in a switch statement would turn the switch statement into a if-else. That is pointless, so allowing an implicit compare-then-assign in switch statements is problem-free. Coincidentally, GCC's -Wparentheses does not apply to switch statements either. Finally, this considers all comparison operators, rather than just the < operator used in the examples in this commit message. The CodeQL check was written by Grok 4.5 Build Beta after several iterations of prompt engineering and follow-up prompts to give it corrections. It has also been subjected to a test suite of 19 true positives and 17 true negatives to verify its behavior. It successfully detected all true positives and fails to detect any true negatives. It has also been applied not only to the OpenZFS codebase, but also the Linux kernel and curl codebases, where it had zero detections. Related queries in CodeQL were also run against the test suite, but had zero detections. The query appears to be a well made query that has a very high signal-to-noise ratio. It might be worth submitting to upstream CodeQL for inclusion, but I would rather add it to our own repository so we can begin benefiting from it today. Assisted-by: Grok 4.5 Build Beta Signed-off-by: Richard Yao --- .github/codeql-cpp.yml | 1 + .../cpp/AssignmentOfComparisonAsCondition.ql | 97 +++++++++++++++++++ 2 files changed, 98 insertions(+) create mode 100644 .github/codeql/custom-queries/cpp/AssignmentOfComparisonAsCondition.ql diff --git a/.github/codeql-cpp.yml b/.github/codeql-cpp.yml index d99cdb559244..15a5a68ae67b 100644 --- a/.github/codeql-cpp.yml +++ b/.github/codeql-cpp.yml @@ -3,3 +3,4 @@ name: "Custom CodeQL Analysis" queries: - uses: ./.github/codeql/custom-queries/cpp/deprecatedFunctionUsage.ql - uses: ./.github/codeql/custom-queries/cpp/dslDatasetHoldReleMismatch.ql + - uses: ./.github/codeql/custom-queries/cpp/AssignmentOfComparisonAsCondition.ql diff --git a/.github/codeql/custom-queries/cpp/AssignmentOfComparisonAsCondition.ql b/.github/codeql/custom-queries/cpp/AssignmentOfComparisonAsCondition.ql new file mode 100644 index 000000000000..1f984f8a105b --- /dev/null +++ b/.github/codeql/custom-queries/cpp/AssignmentOfComparisonAsCondition.ql @@ -0,0 +1,97 @@ +/** + * @name Ambiguous assignment-of-comparison in a condition + * @description Finds any assignment (`=`, `+=`, `-=`, `|=`, …) that appears in + * a branching condition and whose rvalue is an unparenthesized + * comparison. + * + * Comparison binds tighter than assignment, so + * `if ((x = foo() < 0))` / `if ((x += foo() < 0))` + * assigns the boolean result of the comparison. Programmers often + * meant + * `if ((x = foo()) < 0)` / `if ((x += foo()) < 0)` + * (assign/update then compare). GCC `-Wparentheses` is silenced by + * the outer parentheses in both forms, so the compiler does not + * catch the mistake (see openzfs/zfs#18874). + * + * Rule (intentionally simple): + * assignment in a branch condition + * AND rvalue is a comparison + * AND that comparison is not parenthesized. + * + * That is enough: + * - Explicit assign-then-compare `((x = foo()) < 0)` has no + * comparison as the assignment's rvalue. + * - Explicit compare-then-assign `((x = (foo() < 0)))` has a + * parenthesized comparison as the rvalue. + * - Parentheses that only wrap a subexpression of the comparison + * (e.g. `(foo())`) do not count as parenthesizing the + * comparison itself. + * @kind problem + * @problem.severity error + * @precision high + * @id cpp/ambiguous-assignment-of-comparison-in-condition + * @tags reliability + * correctness + * external/cwe/cwe-783 + * external/cwe/cwe-480 + */ + +import cpp + +/** + * The condition expression of a branching construct + * (if / while / do-while / for / ternary `?:`). + */ +Expr branchCondition() { + result = any(IfStmt s).getCondition() + or + result = any(WhileStmt s).getCondition() + or + result = any(DoStmt s).getCondition() + or + result = any(ForStmt s).getCondition() + or + result = any(ConditionalExpr c).getCondition() +} + +/** + * Holds if `e` is the branch condition, or appears anywhere under it + * (including under `&&` / `||` / `!`, and through parentheses/conversions). + */ +predicate inBranchCondition(Expr e) { + e = branchCondition() + or + // Structural child of something already in a branch condition + exists(Expr parent | + parent.getAChild() = e and + inBranchCondition(parent) + ) + or + // ParenthesisExpr and other conversions hang off getConversion(), not getAChild() + exists(Expr base | + base.getConversion+() = e and + inBranchCondition(base) + ) + or + exists(Expr conv | + e.getConversion+() = conv and + inBranchCondition(conv) + ) +} + +from Assignment a, ComparisonOperation cmp +where + // Core rule: assignment in a branch condition whose rvalue is an + // unparenthesized comparison. Covers `=` and all compound forms (`+=`, …) + // because Assignment is the common base class. + a.getRValue() = cmp and + not cmp.isParenthesised() and + inBranchCondition(a) and + // Skip AST from templates that were never instantiated (incomplete / non-code). + not a.isFromUninstantiatedTemplate(_) +select a, + "Assignment (`" + a.getOperator() + + "`) in a branch condition has an unparenthesized comparison as its rvalue; " + + "likely meant `((x " + a.getOperator() + " expr) op val)` rather than " + + "`(x " + a.getOperator() + " expr op val)`. GCC -Wparentheses is silenced " + + "by outer parentheses in both cases."