Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1 +1 @@

ql/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql
Original file line number Diff line number Diff line change
@@ -1 +1 @@

ql/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ ql/cpp/ql/src/Likely Bugs/InconsistentCheckReturnNull.ql
ql/cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.ql
ql/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql
ql/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql
ql/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql
ql/cpp/ql/src/Likely Bugs/Likely Typos/AssignWhereCompareMeant.ql
ql/cpp/ql/src/Likely Bugs/Likely Typos/CompareWhereAssignMeant.ql
ql/cpp/ql/src/Likely Bugs/Likely Typos/DubiousNullCheck.ql
Expand Down
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;
}
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() &lt; 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>
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
not assignment.isUnevaluated() and
not assignment.isFromUninstantiatedTemplate(_)
select assignment,
"The '" + assignment.getOperator() +
"' operation assigns the result of an unparenthesized comparison used in a condition."
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.
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. |
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
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;
}
}
Loading
Loading