diff --git a/cpp/ql/integration-tests/query-suite/cpp-code-quality-extended.qls.expected b/cpp/ql/integration-tests/query-suite/cpp-code-quality-extended.qls.expected index 8b137891791f..85790a23a836 100644 --- a/cpp/ql/integration-tests/query-suite/cpp-code-quality-extended.qls.expected +++ b/cpp/ql/integration-tests/query-suite/cpp-code-quality-extended.qls.expected @@ -1 +1 @@ - +ql/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql diff --git a/cpp/ql/integration-tests/query-suite/cpp-code-quality.qls.expected b/cpp/ql/integration-tests/query-suite/cpp-code-quality.qls.expected index 8b137891791f..85790a23a836 100644 --- a/cpp/ql/integration-tests/query-suite/cpp-code-quality.qls.expected +++ b/cpp/ql/integration-tests/query-suite/cpp-code-quality.qls.expected @@ -1 +1 @@ - +ql/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql diff --git a/cpp/ql/integration-tests/query-suite/cpp-security-and-quality.qls.expected b/cpp/ql/integration-tests/query-suite/cpp-security-and-quality.qls.expected index cb4e5f7b305a..17ebac8ee50c 100644 --- a/cpp/ql/integration-tests/query-suite/cpp-security-and-quality.qls.expected +++ b/cpp/ql/integration-tests/query-suite/cpp-security-and-quality.qls.expected @@ -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 diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.cpp b/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.cpp new file mode 100644 index 000000000000..bf4e1ffa240e --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.cpp @@ -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; +} diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.qhelp b/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.qhelp new file mode 100644 index 000000000000..53c001212001 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.qhelp @@ -0,0 +1,31 @@ + + + + +

Assignment operators have lower precedence than comparison operators. For example, +status = read_status() < 0 assigns the comparison result (zero or one) to +status. This can be unintended when the programmer meant to assign the return value +first and then compare it with zero.

+
+ + +

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.

+
+ + +

In the first condition, status receives either zero or one instead of the value +returned by read_status. The second condition explicitly performs the assignment +before the comparison.

+ +
+ + +
  • SEI CERT C Coding Standard: EXP00-C. Use parentheses for precedence of operation.
  • +
  • C++ reference: Operator precedence.
  • +
    + +
    diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql b/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql new file mode 100644 index 000000000000..4866e2a7bc2d --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql @@ -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." diff --git a/cpp/ql/src/change-notes/2026-08-13-ambiguous-assignment-of-comparison.md b/cpp/ql/src/change-notes/2026-08-13-ambiguous-assignment-of-comparison.md new file mode 100644 index 000000000000..ae9e2c5d3823 --- /dev/null +++ b/cpp/ql/src/change-notes/2026-08-13-ambiguous-assignment-of-comparison.md @@ -0,0 +1,5 @@ +--- +category: newQuery +--- +* Added a new query, `cpp/ambiguous-assignment-of-comparison`, to detect assignments of + unparenthesized comparison results in conditions. diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/AmbiguousAssignmentOfComparison.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/AmbiguousAssignmentOfComparison.expected new file mode 100644 index 000000000000..9075768e9f98 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/AmbiguousAssignmentOfComparison.expected @@ -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. | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/AmbiguousAssignmentOfComparison.qlref b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/AmbiguousAssignmentOfComparison.qlref new file mode 100644 index 000000000000..ffa4d7fb05d8 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/AmbiguousAssignmentOfComparison.qlref @@ -0,0 +1,2 @@ +query: Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/test.c b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/test.c new file mode 100644 index 000000000000..ee78980b3672 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/test.c @@ -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; + } +} diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/test.cpp new file mode 100644 index 000000000000..8082c7925f92 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/test.cpp @@ -0,0 +1,300 @@ +int get_value(); +int get_other_value(); +void *get_pointer(); +bool identity(bool value); + +int direct_if() { + int value; + if ((value = get_value() < 0)) // $ Alert // BAD + return value; + return 0; +} + +int direct_while() { + int value; + while ((value = get_value() != 0)) // $ Alert // BAD + return value; + return 0; +} + +int without_outer_parentheses() { + int value; + if (value = get_value() < 0) // $ Alert // BAD + return value; + return 0; +} + +int for_condition() { + int value; + for (; (value = get_other_value() <= 0);) // $ Alert // BAD + return value; + return 0; +} + +int logical_and() { + int value; + if ((value = get_value() < 0) && get_other_value()) // $ Alert // BAD + return value; + return 0; +} + +int ternary_condition() { + int value; + return (value = get_value() > 0) ? value : 0; // $ Alert // BAD +} + +int pointer_comparison() { + int value; + if ((value = get_pointer() == 0)) // $ Alert // BAD + return value; + return 0; +} + +int parenthesized_operand_only() { + int value; + if ((value = (get_value()) < 0)) // $ Alert // BAD + return value; + return 0; +} + +int do_while_condition() { + int value = 0; + do { + value++; + } while ((value = get_value() < 0)); // $ Alert // BAD + return value; +} + +int logical_or() { + int value; + if (get_other_value() || (value = get_value() > 0)) // $ Alert // BAD + return value; + return 0; +} + +int nested_comparison() { + int value; + if ((value = get_value() < 0) == 1) // $ Alert // BAD + return value; + return 0; +} + +int compound_shift() { + int value = 1; + if ((value <<= get_other_value() > 0)) // $ Alert // BAD + return value; + return 0; +} + +int under_logical_not() { + int value; + if (!(value = get_value() < 0)) // $ Alert // BAD + return value; + return 0; +} + +#define CHECK_VALUE(VALUE) \ + if (((VALUE) = get_value() < 0)) return (VALUE) + +int macro_condition() { + int value; + CHECK_VALUE(value); // $ Alert // BAD + return 0; +} + +int explicit_assign_then_compare() { + int value; + if ((value = get_value()) < 0) // GOOD + return value; + return 0; +} + +int explicit_compare_then_assign() { + int value; + if ((value = (get_value() < 0))) // GOOD + return value; + return 0; +} + +int parenthesized_simple_assignment() { + int value; + if ((value = get_value())) // GOOD + return value; + return 0; +} + +int assignment_outside_condition() { + int value; + value = get_value() < 0; // GOOD: The assignment is not part of a condition. + return value; +} + +int plain_comparison() { + int value = get_value(); + if (value < 0) // GOOD + return value; + return 0; +} + +int explicit_compare_then_assign_without_outer_parentheses() { + int value; + if (value = (get_value() < 0)) // GOOD + return value; + return 0; +} + +int explicit_assign_then_compare_in_for() { + int value; + for (; (value = get_other_value()) >= 0;) // GOOD + return value; + return 0; +} + +int two_explicit_assignments() { + int left, right; + if ((left = get_value()) < 0 && (right = get_other_value()) < 0) // GOOD + return left + right; + return 0; +} + +int explicit_comparison_then_compound_assign() { + int value = 0; + if ((value += (get_value() < 0))) // GOOD + return value; + return 0; +} + +int compound_assignment_without_comparison() { + int value = ~0; + if ((value &= get_value())) // GOOD + return value; + return 0; +} + +int switch_expression() { + int value; + switch (value = get_value() < 0) { // GOOD: This query only covers branching conditions. + case 0: + return value; + default: + return 0; + } +} + +#define EXPLICIT_CHECK(VALUE) \ + if (((VALUE) = get_value()) < 0) return (VALUE) + +int explicit_macro_condition() { + int value; + EXPLICIT_CHECK(value); // GOOD + return 0; +} + +template +int never_instantiated_template(T input) { + int value; + if ((value = input < 0)) // GOOD: Uninstantiated template code is excluded. + return value; + return 0; +} + +int unevaluated_assignment() { + int value; + if (sizeof(value = get_value() < 0)) // GOOD: The assignment is unevaluated. + return value; + return 0; +} + +int constant_assignment() { + int value; + if ((value = 0)) // GOOD: The right-hand side is not a comparison. + return value; + return 0; +} + +int explicit_compound_shift_then_compare() { + int value = 1; + if ((value <<= get_other_value()) > 0) // GOOD + return value; + return 0; +} + +int boolean_result_assignment() { + bool negative; + if ((negative = get_value() < 0)) // GOOD: Assigning a comparison result to a Boolean is natural. + return negative; + return 0; +} + +int boolean_result_compound_assignment() { + bool seen = false; + if ((seen |= get_value() < 0)) // GOOD: Accumulating a comparison result in a Boolean is natural. + return seen; + return 0; +} + +int explicit_static_cast_of_comparison() { + int value; + if ((value = static_cast(get_value() < 0))) // GOOD: The cast explicitly groups the comparison. + return value; + return 0; +} + +int explicit_functional_cast_of_comparison() { + int value; + if ((value = int(get_value() < 0))) // GOOD: The cast explicitly groups the comparison. + return value; + return 0; +} + +template +int instantiated_template_body(T input) { + int value; + if ((value = input < 0)) // $ Alert // BAD + return value; + return 0; +} + +int instantiate_template() { + return instantiated_template_body(get_value()); +} + +struct Comparable { + int value; +}; + +bool operator<(Comparable left, int right) { + return left.value < right; +} + +int overloaded_comparison() { + Comparable input = {get_value()}; + int value; + if ((value = input < 0)) // $ MISSING: Alert // BAD [NOT DETECTED]: overloaded operators are outside this query's scope. + return value; + return 0; +} + +int assignment_as_call_argument() { + int value; + if (identity(value = get_value() < 0)) // $ Alert // BAD: The ambiguous syntax is still in a condition. + return value; + return 0; +} + +int discarded_assignment_in_comma_expression() { + int value; + if ((value = get_value() < 0, get_other_value())) // $ Alert // BAD: Still ambiguous syntax in a condition. + return value; + return 0; +} + +int assignment_in_lambda_body() { + int value; + if ([&]() { + value = get_value() < 0; // GOOD: The lambda body is not the surrounding condition. + return true; + }()) + return value; + return 0; +}