Skip to content

Commit 8f0ea61

Browse files
theinfosecguytheinfosecguy
committed
C/C++: Detect ambiguous assignment of comparison results
Original-patch-by: Richard Yao <richard@ryao.dev>
1 parent 56df417 commit 8f0ea61

11 files changed

Lines changed: 524 additions & 2 deletions

‎cpp/ql/integration-tests/query-suite/cpp-code-quality-extended.qls.expected‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
1+
ql/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql

‎cpp/ql/integration-tests/query-suite/cpp-code-quality.qls.expected‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
1+
ql/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql

‎cpp/ql/integration-tests/query-suite/cpp-security-and-quality.qls.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ ql/cpp/ql/src/Likely Bugs/InconsistentCheckReturnNull.ql
6565
ql/cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.ql
6666
ql/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql
6767
ql/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql
68+
ql/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql
6869
ql/cpp/ql/src/Likely Bugs/Likely Typos/AssignWhereCompareMeant.ql
6970
ql/cpp/ql/src/Likely Bugs/Likely Typos/CompareWhereAssignMeant.ql
7071
ql/cpp/ql/src/Likely Bugs/Likely Typos/DubiousNullCheck.ql

‎cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.cpp‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
int read_status();
2+
3+
int check_status() {
4+
int status;
5+
if (status = read_status() < 0) // BAD: assigns the comparison result.
6+
return status;
7+
8+
if ((status = read_status()) < 0) // GOOD: assigns first, then compares.
9+
return status;
10+
11+
return 0;
12+
}

‎cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.qhelp‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>Assignment operators have lower precedence than comparison operators. For example,
8+
<code>status = read_status() &lt; 0</code> assigns the comparison result (zero or one) to
9+
<code>status</code>. This can be unintended when the programmer meant to assign the return value
10+
first and then compare it with zero.</p>
11+
</overview>
12+
13+
<recommendation>
14+
<p>Use parentheses to make the intended operation order explicit. To assign first and compare the
15+
assigned value, parenthesize the assignment. To intentionally assign the comparison result,
16+
parenthesize the comparison. An explicit cast around the comparison also makes that order clear.</p>
17+
</recommendation>
18+
19+
<example>
20+
<p>In the first condition, <code>status</code> receives either zero or one instead of the value
21+
returned by <code>read_status</code>. The second condition explicitly performs the assignment
22+
before the comparison.</p>
23+
<sample src="AmbiguousAssignmentOfComparison.cpp" />
24+
</example>
25+
26+
<references>
27+
<li>SEI CERT C Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/EXP00-C.+Use+parentheses+for+precedence+of+operation">EXP00-C. Use parentheses for precedence of operation</a>.</li>
28+
<li>C++ reference: <a href="https://en.cppreference.com/w/cpp/language/operator_precedence.html">Operator precedence</a>.</li>
29+
</references>
30+
31+
</qhelp>

‎cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/**
2+
* @name Ambiguous assignment of comparison in condition
3+
* @description Assigning the result of an unparenthesized comparison in a condition may indicate
4+
* that the assignment and comparison are grouped incorrectly.
5+
* @kind problem
6+
* @problem.severity warning
7+
* @precision high
8+
* @id cpp/ambiguous-assignment-of-comparison
9+
* @tags quality
10+
* reliability
11+
* correctness
12+
* external/cwe/cwe-783
13+
*/
14+
15+
import cpp
16+
17+
/** Gets a condition that controls branching. */
18+
private Expr getACondition() {
19+
result = any(IfStmt s).getCondition()
20+
or
21+
result = any(Loop s).getCondition()
22+
or
23+
result = any(ConditionalExpr e).getCondition()
24+
}
25+
26+
/**
27+
* Holds if `assignment` occurs within a condition that controls branching.
28+
*
29+
* This includes nested expressions, such as function arguments and either operand of a comma
30+
* expression, because the ambiguous syntax still occurs within the condition.
31+
*/
32+
private predicate occursInCondition(Assignment assignment) {
33+
assignment.getParent*() = getACondition()
34+
}
35+
36+
/**
37+
* Holds if `comparison` is explicitly grouped using parentheses or an explicit cast.
38+
*/
39+
private predicate isExplicitlyGrouped(ComparisonOperation comparison) {
40+
comparison.isParenthesised()
41+
or
42+
exists(Cast cast | cast = comparison.getConversion+() and not cast.isImplicit())
43+
}
44+
45+
from Assignment assignment, ComparisonOperation comparison
46+
where
47+
assignment.getRValue() = comparison and
48+
not isExplicitlyGrouped(comparison) and
49+
occursInCondition(assignment) and
50+
// Assigning a comparison result to a Boolean is normally intentional.
51+
not assignment.getLValue().getUnspecifiedType() instanceof BoolType and
52+
not assignment.isUnevaluated() and
53+
not assignment.isFromUninstantiatedTemplate(_)
54+
select assignment,
55+
"The '" + assignment.getOperator() +
56+
"' operation assigns the result of an unparenthesized comparison used in a condition."

‎cpp/ql/src/change-notes/2026-08-13-ambiguous-assignment-of-comparison.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `cpp/ambiguous-assignment-of-comparison`, to detect assignments of
5+
unparenthesized comparison results in conditions.

‎cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/AmbiguousAssignmentOfComparison.expected‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
| test.c:6:8:6:31 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
2+
| test.c:13:30:13:54 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
3+
| test.c:20:8:20:33 | ... /= ... | The '/=' operation assigns the result of an unparenthesized comparison used in a condition. |
4+
| test.c:27:8:27:33 | ... %= ... | The '%=' operation assigns the result of an unparenthesized comparison used in a condition. |
5+
| test.c:34:8:34:32 | ... \|= ... | The '\|=' operation assigns the result of an unparenthesized comparison used in a condition. |
6+
| test.c:41:8:41:33 | ... >>= ... | The '>>=' operation assigns the result of an unparenthesized comparison used in a condition. |
7+
| test.c:51:3:51:26 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
8+
| test.cpp:8:8:8:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
9+
| test.cpp:15:11:15:34 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
10+
| test.cpp:22:7:22:29 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
11+
| test.cpp:29:11:29:40 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
12+
| test.cpp:36:8:36:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
13+
| test.cpp:43:11:43:33 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
14+
| test.cpp:48:8:48:33 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
15+
| test.cpp:55:8:55:32 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
16+
| test.cpp:64:13:64:35 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
17+
| test.cpp:70:29:70:51 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
18+
| test.cpp:77:8:77:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
19+
| test.cpp:84:8:84:38 | ... <<= ... | The '<<=' operation assigns the result of an unparenthesized comparison used in a condition. |
20+
| test.cpp:91:9:91:31 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
21+
| test.cpp:101:3:101:20 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
22+
| test.cpp:253:8:253:24 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
23+
| test.cpp:280:16:280:38 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |
24+
| test.cpp:287:8:287:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. |

‎cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/AmbiguousAssignmentOfComparison.qlref‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
query: Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql
2+
postprocess: utils/test/InlineExpectationsTestQuery.ql

‎cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/test.c‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
int read_value(void);
2+
int read_other_value(void);
3+
4+
int c_direct_condition(void) {
5+
int value;
6+
if ((value = read_value() < 0)) // $ Alert // BAD
7+
return value;
8+
return 0;
9+
}
10+
11+
int c_logical_condition(void) {
12+
int value;
13+
if (read_other_value() && (value = read_value() >= 0)) // $ Alert // BAD
14+
return value;
15+
return 0;
16+
}
17+
18+
int c_compound_divide(void) {
19+
int value = 8;
20+
if ((value /= read_value() != 0)) // $ Alert // BAD
21+
return value;
22+
return 0;
23+
}
24+
25+
int c_compound_remainder(void) {
26+
int value = 8;
27+
if ((value %= read_value() != 0)) // $ Alert // BAD
28+
return value;
29+
return 0;
30+
}
31+
32+
int c_compound_bitwise_or(void) {
33+
int value = 0;
34+
if ((value |= read_value() > 0)) // $ Alert // BAD
35+
return value;
36+
return 0;
37+
}
38+
39+
int c_compound_right_shift(void) {
40+
int value = 8;
41+
if ((value >>= read_value() > 0)) // $ Alert // BAD
42+
return value;
43+
return 0;
44+
}
45+
46+
#define C_AMBIGUOUS_CHECK(VALUE) \
47+
if (((VALUE) = read_value() < 0)) return (VALUE)
48+
49+
int c_macro_condition(void) {
50+
int value;
51+
C_AMBIGUOUS_CHECK(value); // $ Alert // BAD
52+
return 0;
53+
}
54+
55+
int c_explicit_assign_then_compare(void) {
56+
int value;
57+
if ((value = read_value()) < 0) // GOOD
58+
return value;
59+
return 0;
60+
}
61+
62+
int c_explicit_compare_then_assign(void) {
63+
int value;
64+
if ((value = (read_value() < 0))) // GOOD
65+
return value;
66+
return 0;
67+
}
68+
69+
int c_explicit_cast_of_comparison(void) {
70+
int value;
71+
if ((value = (int)(read_value() < 0))) // GOOD: The cast explicitly groups the comparison.
72+
return value;
73+
return 0;
74+
}
75+
76+
int c_boolean_result_assignment(void) {
77+
_Bool negative;
78+
if ((negative = read_value() < 0)) // GOOD: Assigning a comparison result to a Boolean is natural.
79+
return negative;
80+
return 0;
81+
}
82+
83+
int c_switch_expression(void) {
84+
int value;
85+
switch (value = read_value() < 0) { // GOOD: This query only covers branching conditions.
86+
case 0:
87+
return value;
88+
default:
89+
return 0;
90+
}
91+
}

0 commit comments

Comments
 (0)