C/C++: Detect ambiguous assignment of comparison results #22336
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| ql/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| ql/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| int read_status(); | ||
|
|
||
| int check_status() { | ||
| int status; | ||
| if ((status = read_status() < 0)) // BAD: assigns the comparison result. | ||
| return status; | ||
|
|
||
| if ((status = read_status()) < 0) // GOOD: assigns first, then compares. | ||
| return status; | ||
|
|
||
| return 0; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
| <overview> | ||
| <p>Assignment operators have lower precedence than comparison operators. For example, | ||
| <code>status = read_status() < 0</code> assigns the comparison result (zero or one) to | ||
| <code>status</code>. This can be unintended when the programmer meant to assign the return value | ||
| first and then compare it with zero.</p> | ||
| </overview> | ||
|
|
||
| <recommendation> | ||
| <p>Use parentheses to make the intended operation order explicit. To assign first and compare the | ||
| assigned value, parenthesize the assignment. To intentionally assign the comparison result, | ||
| parenthesize the comparison. An explicit cast around the comparison also makes that order clear.</p> | ||
| </recommendation> | ||
|
|
||
| <example> | ||
| <p>In the first condition, <code>status</code> receives either zero or one instead of the value | ||
| returned by <code>read_status</code>. The second condition explicitly performs the assignment | ||
| before the comparison.</p> | ||
| <sample src="AmbiguousAssignmentOfComparison.cpp" /> | ||
| </example> | ||
|
|
||
| <references> | ||
| <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> | ||
| <li>C++ reference: <a href="https://en.cppreference.com/w/cpp/language/operator_precedence.html">Operator precedence</a>.</li> | ||
| </references> | ||
|
|
||
| </qhelp> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| /** | ||
| * @name Ambiguous assignment of comparison in condition | ||
| * @description Assigning the result of an unparenthesized comparison in a condition may indicate | ||
| * that the assignment and comparison are grouped incorrectly. | ||
| * @kind problem | ||
| * @problem.severity warning | ||
| * @precision high | ||
| * @id cpp/ambiguous-assignment-of-comparison | ||
| * @tags quality | ||
| * reliability | ||
| * correctness | ||
| * external/cwe/cwe-783 | ||
| */ | ||
|
|
||
| import cpp | ||
|
|
||
| /** Gets a condition that controls branching. */ | ||
| private Expr getACondition() { | ||
| 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 e).getCondition() | ||
| } | ||
|
|
||
| /** | ||
| * Holds if `assignment` occurs within a condition that controls branching. | ||
| * | ||
| * This includes nested expressions, such as function arguments and either operand of a comma | ||
| * expression, because the ambiguous syntax still occurs within the condition. | ||
| */ | ||
| private predicate occursInCondition(Assignment assignment) { | ||
| assignment.getParent*() = getACondition() | ||
| } | ||
|
|
||
| /** | ||
| * Holds if `comparison` is explicitly grouped using parentheses or an explicit cast. | ||
| */ | ||
| private predicate isExplicitlyGrouped(ComparisonOperation comparison) { | ||
| comparison.isParenthesised() | ||
| or | ||
| exists(Cast cast | cast = comparison.getConversion+() and not cast.isImplicit()) | ||
| } | ||
|
|
||
| from Assignment assignment, ComparisonOperation comparison | ||
| where | ||
| assignment.getRValue() = comparison and | ||
| not isExplicitlyGrouped(comparison) and | ||
| occursInCondition(assignment) and | ||
| // Assigning a comparison result to a Boolean is normally intentional. | ||
| not assignment.getLValue().getUnspecifiedType() instanceof BoolType and | ||
|
There was a problem hiding this comment. While this is normally intentional, I believe |
||
| not assignment.isUnevaluated() and | ||
| not assignment.isFromUninstantiatedTemplate(_) | ||
| select assignment, | ||
| "The '" + assignment.getOperator() + | ||
| "' operation assigns the result of an unparenthesized comparison used in a condition." | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| category: newQuery | ||
| --- | ||
| * Added a new query, `cpp/ambiguous-assignment-of-comparison`, to detect assignments of | ||
| unparenthesized comparison results in conditions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| | test.c:6:8:6:31 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.c:13:30:13:54 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.c:20:8:20:33 | ... /= ... | The '/=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.c:27:8:27:33 | ... %= ... | The '%=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.c:34:8:34:32 | ... \|= ... | The '\|=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.c:41:8:41:33 | ... >>= ... | The '>>=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.c:51:3:51:26 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:8:8:8:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:15:11:15:34 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:22:7:22:29 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:29:11:29:40 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:36:8:36:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:43:11:43:33 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:48:8:48:33 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:55:8:55:32 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:64:13:64:35 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:70:29:70:51 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:77:8:77:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:84:8:84:38 | ... <<= ... | The '<<=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:91:9:91:31 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:101:3:101:20 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:253:8:253:24 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:280:16:280:38 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | | ||
| | test.cpp:287:8:287:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison used in a condition. | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| query: Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql | ||
| postprocess: utils/test/InlineExpectationsTestQuery.ql |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| int read_value(void); | ||
| int read_other_value(void); | ||
|
|
||
| int c_direct_condition(void) { | ||
| int value; | ||
| if ((value = read_value() < 0)) // $ Alert // BAD | ||
| return value; | ||
| return 0; | ||
| } | ||
|
|
||
| int c_logical_condition(void) { | ||
| int value; | ||
| if (read_other_value() && (value = read_value() >= 0)) // $ Alert // BAD | ||
| return value; | ||
| return 0; | ||
| } | ||
|
|
||
| int c_compound_divide(void) { | ||
| int value = 8; | ||
| if ((value /= read_value() != 0)) // $ Alert // BAD | ||
| return value; | ||
| return 0; | ||
| } | ||
|
|
||
| int c_compound_remainder(void) { | ||
| int value = 8; | ||
| if ((value %= read_value() != 0)) // $ Alert // BAD | ||
| return value; | ||
| return 0; | ||
| } | ||
|
|
||
| int c_compound_bitwise_or(void) { | ||
| int value = 0; | ||
| if ((value |= read_value() > 0)) // $ Alert // BAD | ||
| return value; | ||
| return 0; | ||
| } | ||
|
|
||
| int c_compound_right_shift(void) { | ||
| int value = 8; | ||
| if ((value >>= read_value() > 0)) // $ Alert // BAD | ||
| return value; | ||
| return 0; | ||
| } | ||
|
|
||
| #define C_AMBIGUOUS_CHECK(VALUE) \ | ||
| if (((VALUE) = read_value() < 0)) return (VALUE) | ||
|
|
||
| int c_macro_condition(void) { | ||
| int value; | ||
| C_AMBIGUOUS_CHECK(value); // $ Alert // BAD | ||
| return 0; | ||
| } | ||
|
|
||
| int c_explicit_assign_then_compare(void) { | ||
| int value; | ||
| if ((value = read_value()) < 0) // GOOD | ||
| return value; | ||
| return 0; | ||
| } | ||
|
|
||
| int c_explicit_compare_then_assign(void) { | ||
| int value; | ||
| if ((value = (read_value() < 0))) // GOOD | ||
| return value; | ||
| return 0; | ||
| } | ||
|
|
||
| int c_explicit_cast_of_comparison(void) { | ||
| int value; | ||
| if ((value = (int)(read_value() < 0))) // GOOD: The cast explicitly groups the comparison. | ||
| return value; | ||
| return 0; | ||
| } | ||
|
|
||
| int c_boolean_result_assignment(void) { | ||
| _Bool negative; | ||
| if ((negative = read_value() < 0)) // GOOD: Assigning a comparison result to a Boolean is natural. | ||
| return negative; | ||
| return 0; | ||
| } | ||
|
|
||
| int c_switch_expression(void) { | ||
| int value; | ||
| switch (value = read_value() < 0) { // GOOD: This query only covers branching conditions. | ||
| case 0: | ||
| return value; | ||
| default: | ||
| return 0; | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the only good variant. The following is good too, because it shows that the developer really meant it.