1/** 2 * @name Ambiguous assignment-of-comparison in a condition 3 * @description Finds any assignment (`=`, `+=`, `-=`, `|=`, …) that appears in 4 * a branching condition and whose rvalue is an unparenthesized 5 * comparison. 6 * 7 * Comparison binds tighter than assignment, so 8 * `if ((x = foo() < 0))` / `if ((x += foo() < 0))` 9 * assigns the boolean result of the comparison. Programmers often 10 * meant 11 * `if ((x = foo()) < 0)` / `if ((x += foo()) < 0)` 12 * (assign/update then compare). GCC `-Wparentheses` is silenced by 13 * the outer parentheses in both forms, so the compiler does not 14 * catch the mistake (see openzfs/zfs#18874). 15 * 16 * Rule (intentionally simple): 17 * assignment in a branch condition 18 * AND rvalue is a comparison 19 * AND that comparison is not parenthesized. 20 * 21 * That is enough: 22 * - Explicit assign-then-compare `((x = foo()) < 0)` has no 23 * comparison as the assignment's rvalue. 24 * - Explicit compare-then-assign `((x = (foo() < 0)))` has a 25 * parenthesized comparison as the rvalue. 26 * - Parentheses that only wrap a subexpression of the comparison 27 * (e.g. `(foo())`) do not count as parenthesizing the 28 * comparison itself. 29 * @kind problem 30 * @problem.severity error 31 * @precision high 32 * @id cpp/ambiguous-assignment-of-comparison-in-condition 33 * @tags reliability 34 * correctness 35 * external/cwe/cwe-783 36 * external/cwe/cwe-480 37 */ 38 39import cpp 40 41/** 42 * The condition expression of a branching construct 43 * (if / while / do-while / for / ternary `?:`). 44 */ 45Expr branchCondition() { 46 result = any(IfStmt s).getCondition() 47 or 48 result = any(WhileStmt s).getCondition() 49 or 50 result = any(DoStmt s).getCondition() 51 or 52 result = any(ForStmt s).getCondition() 53 or 54 result = any(ConditionalExpr c).getCondition() 55} 56 57/** 58 * Holds if `e` is the branch condition, or appears anywhere under it 59 * (including under `&&` / `||` / `!`, and through parentheses/conversions). 60 */ 61predicate inBranchCondition(Expr e) { 62 e = branchCondition() 63 or 64 // Structural child of something already in a branch condition 65 exists(Expr parent | 66 parent.getAChild() = e and 67 inBranchCondition(parent) 68 ) 69 or 70 // ParenthesisExpr and other conversions hang off getConversion(), not getAChild() 71 exists(Expr base | 72 base.getConversion+() = e and 73 inBranchCondition(base) 74 ) 75 or 76 exists(Expr conv | 77 e.getConversion+() = conv and 78 inBranchCondition(conv) 79 ) 80} 81 82from Assignment a, ComparisonOperation cmp 83where 84 // Core rule: assignment in a branch condition whose rvalue is an 85 // unparenthesized comparison. Covers `=` and all compound forms (`+=`, …) 86 // because Assignment is the common base class. 87 a.getRValue() = cmp and 88 not cmp.isParenthesised() and 89 inBranchCondition(a) and 90 // Skip AST from templates that were never instantiated (incomplete / non-code). 91 not a.isFromUninstantiatedTemplate(_) 92select a, 93 "Assignment (`" + a.getOperator() + 94 "`) in a branch condition has an unparenthesized comparison as its rvalue; " + 95 "likely meant `((x " + a.getOperator() + " expr) op val)` rather than " + 96 "`(x " + a.getOperator() + " expr op val)`." 97