Skip to content

Commit 3715b86

Browse files
theinfosecguytheinfosecguy
committed
Refine truth-value detection
1 parent 8f0ea61 commit 3715b86

5 files changed

Lines changed: 194 additions & 48 deletions

File tree

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

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
/**
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.
2+
* @name Ambiguous assignment of comparison used as truth value
3+
* @description Assigning the result of an unparenthesized comparison when the assignment is used
4+
* as a truth value may indicate that the assignment and comparison are grouped
5+
* incorrectly.
56
* @kind problem
67
* @problem.severity warning
78
* @precision high
@@ -14,23 +15,36 @@
1415

1516
import cpp
1617

17-
/** Gets a condition that controls branching. */
18-
private Expr getACondition() {
19-
result = any(IfStmt s).getCondition()
18+
/** Holds if the value of `expression` is directly used as a truth value. */
19+
private predicate isDirectlyUsedAsTruthValue(Expr expression) {
20+
expression.isCondition()
2021
or
21-
result = any(Loop s).getCondition()
22+
expression = any(UnaryLogicalOperation operation).getAnOperand()
2223
or
23-
result = any(ConditionalExpr e).getCondition()
24+
expression = any(BinaryLogicalOperation operation).getAnOperand()
2425
}
2526

2627
/**
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.
28+
* Holds if the value of `expression` is used as a truth value, possibly after contributing to a
29+
* comma, conditional, or comparison expression.
3130
*/
32-
private predicate occursInCondition(Assignment assignment) {
33-
assignment.getParent*() = getACondition()
31+
private predicate isUsedAsTruthValue(Expr expression) {
32+
isDirectlyUsedAsTruthValue(expression)
33+
or
34+
exists(CommaExpr comma |
35+
expression = comma.getRightOperand() and
36+
isUsedAsTruthValue(comma)
37+
)
38+
or
39+
exists(ConditionalExpr conditional |
40+
expression = [conditional.getThen(), conditional.getElse()] and
41+
isUsedAsTruthValue(conditional)
42+
)
43+
or
44+
exists(ComparisonOperation comparison |
45+
expression = comparison.getAnOperand() and
46+
isUsedAsTruthValue(comparison)
47+
)
3448
}
3549

3650
/**
@@ -46,11 +60,13 @@ from Assignment assignment, ComparisonOperation comparison
4660
where
4761
assignment.getRValue() = comparison and
4862
not isExplicitlyGrouped(comparison) and
49-
occursInCondition(assignment) and
50-
// Assigning a comparison result to a Boolean is normally intentional.
63+
isUsedAsTruthValue(assignment) and
64+
// A Boolean lvalue makes assigning the comparison result type-appropriate and normally
65+
// intentional.
5166
not assignment.getLValue().getUnspecifiedType() instanceof BoolType and
5267
not assignment.isUnevaluated() and
5368
not assignment.isFromUninstantiatedTemplate(_)
5469
select assignment,
5570
"The '" + assignment.getOperator() +
56-
"' operation assigns the result of an unparenthesized comparison used in a condition."
71+
"' operation assigns the result of an unparenthesized comparison, and its result is used as " +
72+
"a truth value."

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
category: newQuery
33
---
44
* Added a new query, `cpp/ambiguous-assignment-of-comparison`, to detect assignments of
5-
unparenthesized comparison results in conditions.
5+
unparenthesized comparison results when the assignment is used as a truth value.

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

Lines changed: 36 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,36 @@
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. |
1+
| test.c:6:8:6:31 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
2+
| test.c:13:30:13:54 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
3+
| test.c:20:8:20:33 | ... /= ... | The '/=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
4+
| test.c:27:8:27:33 | ... %= ... | The '%=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
5+
| test.c:34:8:34:32 | ... \|= ... | The '\|=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
6+
| test.c:41:8:41:33 | ... >>= ... | The '>>=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
7+
| test.c:51:3:51:26 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
8+
| test.c:102:28:102:51 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
9+
| test.c:109:15:109:38 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
10+
| test.c:116:49:116:72 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
11+
| test.c:130:11:130:34 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
12+
| test.cpp:8:8:8:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
13+
| test.cpp:15:11:15:34 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
14+
| test.cpp:22:7:22:29 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
15+
| test.cpp:29:11:29:40 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
16+
| test.cpp:36:8:36:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
17+
| test.cpp:43:11:43:33 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
18+
| test.cpp:48:8:48:33 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
19+
| test.cpp:55:8:55:32 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
20+
| test.cpp:64:13:64:35 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
21+
| test.cpp:70:29:70:51 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
22+
| test.cpp:77:8:77:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
23+
| test.cpp:84:8:84:38 | ... <<= ... | The '<<=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
24+
| test.cpp:91:9:91:31 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
25+
| test.cpp:101:3:101:20 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
26+
| test.cpp:253:8:253:24 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
27+
| test.cpp:294:27:294:49 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
28+
| test.cpp:301:27:301:49 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
29+
| test.cpp:308:9:308:31 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
30+
| test.cpp:315:15:315:37 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
31+
| test.cpp:322:23:322:45 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
32+
| test.cpp:329:47:329:69 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
33+
| test.cpp:343:47:343:69 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
34+
| test.cpp:350:11:350:33 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
35+
| test.cpp:355:12:355:34 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |
36+
| test.cpp:360:14:360:36 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. |

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

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,55 @@ int c_boolean_result_assignment(void) {
8282

8383
int c_switch_expression(void) {
8484
int value;
85-
switch (value = read_value() < 0) { // GOOD: This query only covers branching conditions.
85+
switch (value = read_value() < 0) { // GOOD: The switch operand is not used as a truth value.
8686
case 0:
8787
return value;
8888
default:
8989
return 0;
9090
}
9191
}
92+
93+
int c_discarded_assignment_in_comma_expression(void) {
94+
int value;
95+
if ((value = read_value() < 0, read_other_value())) // GOOD: The assignment result is discarded.
96+
return value;
97+
return 0;
98+
}
99+
100+
int c_truth_valued_assignment_in_comma_expression(void) {
101+
int value;
102+
if ((read_other_value(), value = read_value() < 0)) // $ Alert // BAD
103+
return value;
104+
return 0;
105+
}
106+
107+
int c_truth_valued_conditional_then_arm(int flag) {
108+
int value;
109+
if (flag ? (value = read_value() < 0) : 0) // $ Alert // BAD
110+
return value;
111+
return 0;
112+
}
113+
114+
int c_nested_truth_valued_comma_expression(void) {
115+
int value;
116+
if ((read_other_value(), (read_other_value(), value = read_value() < 0))) // $ Alert // BAD
117+
return value;
118+
return 0;
119+
}
120+
121+
int c_nested_discarded_comma_expression(void) {
122+
int value;
123+
if (((read_other_value(), value = read_value() < 0), read_other_value())) // GOOD: Discarded.
124+
return value;
125+
return 0;
126+
}
127+
128+
int c_logical_value_outside_branch(void) {
129+
int value;
130+
return (value = read_value() < 0) && read_other_value(); // $ Alert // BAD
131+
}
132+
133+
int c_returned_assignment(void) {
134+
int value;
135+
return value = read_value() < 0; // GOOD: The assignment result is not used as a truth value.
136+
}

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

Lines changed: 78 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
int get_value();
22
int get_other_value();
33
void *get_pointer();
4-
bool identity(bool value);
4+
bool check(int value);
55

66
int direct_if() {
77
int value;
@@ -74,7 +74,7 @@ int logical_or() {
7474

7575
int nested_comparison() {
7676
int value;
77-
if ((value = get_value() < 0) == 1) // $ Alert // BAD
77+
if ((value = get_value() < 0) == false) // $ Alert // BAD
7878
return value;
7979
return 0;
8080
}
@@ -173,7 +173,7 @@ int compound_assignment_without_comparison() {
173173

174174
int switch_expression() {
175175
int value;
176-
switch (value = get_value() < 0) { // GOOD: This query only covers branching conditions.
176+
switch (value = get_value() < 0) { // GOOD: The switch operand is not used as a truth value.
177177
case 0:
178178
return value;
179179
default:
@@ -277,14 +277,87 @@ int overloaded_comparison() {
277277

278278
int assignment_as_call_argument() {
279279
int value;
280-
if (identity(value = get_value() < 0)) // $ Alert // BAD: The ambiguous syntax is still in a condition.
280+
if (check(value = get_value() < 0)) // GOOD: The assignment only computes an integer argument.
281281
return value;
282282
return 0;
283283
}
284284

285285
int discarded_assignment_in_comma_expression() {
286286
int value;
287-
if ((value = get_value() < 0, get_other_value())) // $ Alert // BAD: Still ambiguous syntax in a condition.
287+
if ((value = get_value() < 0, get_other_value())) // GOOD: The assignment result is discarded.
288+
return value;
289+
return 0;
290+
}
291+
292+
int truth_valued_assignment_in_comma_expression() {
293+
int value;
294+
if ((get_other_value(), value = get_value() < 0)) // $ Alert // BAD
295+
return value;
296+
return 0;
297+
}
298+
299+
int comma_assignment_under_comparison() {
300+
int value;
301+
if ((get_other_value(), value = get_value() < 0) == false) // $ Alert // BAD
302+
return value;
303+
return 0;
304+
}
305+
306+
int nested_truth_valued_comparisons() {
307+
int value;
308+
if (((value = get_value() < 0) == false) == false) // $ Alert // BAD
309+
return value;
310+
return 0;
311+
}
312+
313+
int truth_valued_conditional_then_arm(bool flag) {
314+
int value;
315+
if (flag ? (value = get_value() < 0) : false) // $ Alert // BAD
316+
return value;
317+
return 0;
318+
}
319+
320+
int truth_valued_conditional_else_arm(bool flag) {
321+
int value;
322+
if (flag ? false : (value = get_value() < 0)) // $ Alert // BAD
323+
return value;
324+
return 0;
325+
}
326+
327+
int nested_truth_valued_comma_expression() {
328+
int value;
329+
if ((get_other_value(), (get_other_value(), value = get_value() < 0))) // $ Alert // BAD
330+
return value;
331+
return 0;
332+
}
333+
334+
int nested_discarded_comma_expression() {
335+
int value;
336+
if (((get_other_value(), value = get_value() < 0), get_other_value())) // GOOD: Discarded.
337+
return value;
338+
return 0;
339+
}
340+
341+
int nested_comma_assignment_under_comparison() {
342+
int value;
343+
if ((get_other_value(), (get_other_value(), value = get_value() < 0)) == false) // $ Alert // BAD
344+
return value;
345+
return 0;
346+
}
347+
348+
bool logical_value_outside_branch() {
349+
int value;
350+
return (value = get_value() < 0) && get_other_value(); // $ Alert // BAD
351+
}
352+
353+
bool logical_not_outside_branch() {
354+
int value;
355+
return !(value = get_value() < 0); // $ Alert // BAD
356+
}
357+
358+
int truth_valued_assignment_inside_call_argument() {
359+
int value;
360+
if (check((value = get_value() < 0) && get_other_value())) // $ Alert // BAD
288361
return value;
289362
return 0;
290363
}

0 commit comments

Comments
 (0)