From 8f0ea6147a4de49bebdc6062857486871a881d5b Mon Sep 17 00:00:00 2001 From: Keshav Malik Date: Thu, 13 Aug 2026 16:16:36 +0530 Subject: [PATCH 1/2] C/C++: Detect ambiguous assignment of comparison results Original-patch-by: Richard Yao --- .../cpp-code-quality-extended.qls.expected | 2 +- .../query-suite/cpp-code-quality.qls.expected | 2 +- .../cpp-security-and-quality.qls.expected | 1 + .../AmbiguousAssignmentOfComparison.cpp | 12 + .../AmbiguousAssignmentOfComparison.qhelp | 31 ++ .../AmbiguousAssignmentOfComparison.ql | 56 ++++ ...8-13-ambiguous-assignment-of-comparison.md | 5 + .../AmbiguousAssignmentOfComparison.expected | 24 ++ .../AmbiguousAssignmentOfComparison.qlref | 2 + .../AmbiguousAssignmentOfComparison/test.c | 91 ++++++ .../AmbiguousAssignmentOfComparison/test.cpp | 300 ++++++++++++++++++ 11 files changed, 524 insertions(+), 2 deletions(-) create mode 100644 cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.cpp create mode 100644 cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.qhelp create mode 100644 cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql create mode 100644 cpp/ql/src/change-notes/2026-08-13-ambiguous-assignment-of-comparison.md create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/AmbiguousAssignmentOfComparison.expected create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/AmbiguousAssignmentOfComparison.qlref create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/test.c create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison/test.cpp 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..c99ee284346c --- /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..be3007ffa038 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql @@ -0,0 +1,56 @@ +/** + * @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(Loop 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; +} From 3715b86fc4e7a0b42b695840bf08165024120793 Mon Sep 17 00:00:00 2001 From: Keshav Malik Date: Sun, 23 Aug 2026 14:16:10 +0530 Subject: [PATCH 2/2] Refine truth-value detection --- .../AmbiguousAssignmentOfComparison.ql | 50 +++++++---- ...8-13-ambiguous-assignment-of-comparison.md | 2 +- .../AmbiguousAssignmentOfComparison.expected | 60 ++++++++------ .../AmbiguousAssignmentOfComparison/test.c | 47 ++++++++++- .../AmbiguousAssignmentOfComparison/test.cpp | 83 +++++++++++++++++-- 5 files changed, 194 insertions(+), 48 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql b/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql index be3007ffa038..c767fecac9a8 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql +++ b/cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.ql @@ -1,7 +1,8 @@ /** - * @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. + * @name Ambiguous assignment of comparison used as truth value + * @description Assigning the result of an unparenthesized comparison when the assignment is used + * as a truth value may indicate that the assignment and comparison are grouped + * incorrectly. * @kind problem * @problem.severity warning * @precision high @@ -14,23 +15,36 @@ import cpp -/** Gets a condition that controls branching. */ -private Expr getACondition() { - result = any(IfStmt s).getCondition() +/** Holds if the value of `expression` is directly used as a truth value. */ +private predicate isDirectlyUsedAsTruthValue(Expr expression) { + expression.isCondition() or - result = any(Loop s).getCondition() + expression = any(UnaryLogicalOperation operation).getAnOperand() or - result = any(ConditionalExpr e).getCondition() + expression = any(BinaryLogicalOperation operation).getAnOperand() } /** - * 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. + * Holds if the value of `expression` is used as a truth value, possibly after contributing to a + * comma, conditional, or comparison expression. */ -private predicate occursInCondition(Assignment assignment) { - assignment.getParent*() = getACondition() +private predicate isUsedAsTruthValue(Expr expression) { + isDirectlyUsedAsTruthValue(expression) + or + exists(CommaExpr comma | + expression = comma.getRightOperand() and + isUsedAsTruthValue(comma) + ) + or + exists(ConditionalExpr conditional | + expression = [conditional.getThen(), conditional.getElse()] and + isUsedAsTruthValue(conditional) + ) + or + exists(ComparisonOperation comparison | + expression = comparison.getAnOperand() and + isUsedAsTruthValue(comparison) + ) } /** @@ -46,11 +60,13 @@ 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. + isUsedAsTruthValue(assignment) and + // A Boolean lvalue makes assigning the comparison result type-appropriate and 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." + "' operation assigns the result of an unparenthesized comparison, and its result is used as " + + "a truth value." 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 index ae9e2c5d3823..91e83979b524 100644 --- 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 @@ -2,4 +2,4 @@ category: newQuery --- * Added a new query, `cpp/ambiguous-assignment-of-comparison`, to detect assignments of - unparenthesized comparison results in conditions. + unparenthesized comparison results when the assignment is used as a truth value. 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 index 9075768e9f98..8b905f48bba2 100644 --- 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 @@ -1,24 +1,36 @@ -| 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. | +| test.c:6:8:6:31 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.c:13:30:13:54 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.c:20:8:20:33 | ... /= ... | The '/=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.c:27:8:27:33 | ... %= ... | The '%=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.c:34:8:34:32 | ... \|= ... | The '\|=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.c:41:8:41:33 | ... >>= ... | The '>>=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.c:51:3:51:26 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.c:102:28:102:51 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.c:109:15:109:38 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.c:116:49:116:72 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.c:130:11:130:34 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:8:8:8:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:15:11:15:34 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:22:7:22:29 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:29:11:29:40 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:36:8:36:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:43:11:43:33 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:48:8:48:33 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:55:8:55:32 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:64:13:64:35 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:70:29:70:51 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:77:8:77:30 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:84:8:84:38 | ... <<= ... | The '<<=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:91:9:91:31 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:101:3:101:20 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:253:8:253:24 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:294:27:294:49 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:301:27:301:49 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:308:9:308:31 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:315:15:315:37 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:322:23:322:45 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:329:47:329:69 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:343:47:343:69 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:350:11:350:33 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:355:12:355:34 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | +| test.cpp:360:14:360:36 | ... = ... | The '=' operation assigns the result of an unparenthesized comparison, and its result is used as a truth value. | 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 index ee78980b3672..18e7675e40ad 100644 --- 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 @@ -82,10 +82,55 @@ int c_boolean_result_assignment(void) { int c_switch_expression(void) { int value; - switch (value = read_value() < 0) { // GOOD: This query only covers branching conditions. + switch (value = read_value() < 0) { // GOOD: The switch operand is not used as a truth value. case 0: return value; default: return 0; } } + +int c_discarded_assignment_in_comma_expression(void) { + int value; + if ((value = read_value() < 0, read_other_value())) // GOOD: The assignment result is discarded. + return value; + return 0; +} + +int c_truth_valued_assignment_in_comma_expression(void) { + int value; + if ((read_other_value(), value = read_value() < 0)) // $ Alert // BAD + return value; + return 0; +} + +int c_truth_valued_conditional_then_arm(int flag) { + int value; + if (flag ? (value = read_value() < 0) : 0) // $ Alert // BAD + return value; + return 0; +} + +int c_nested_truth_valued_comma_expression(void) { + int value; + if ((read_other_value(), (read_other_value(), value = read_value() < 0))) // $ Alert // BAD + return value; + return 0; +} + +int c_nested_discarded_comma_expression(void) { + int value; + if (((read_other_value(), value = read_value() < 0), read_other_value())) // GOOD: Discarded. + return value; + return 0; +} + +int c_logical_value_outside_branch(void) { + int value; + return (value = read_value() < 0) && read_other_value(); // $ Alert // BAD +} + +int c_returned_assignment(void) { + int value; + return value = read_value() < 0; // GOOD: The assignment result is not used as a truth value. +} 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 index 8082c7925f92..ac20faf01958 100644 --- 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 @@ -1,7 +1,7 @@ int get_value(); int get_other_value(); void *get_pointer(); -bool identity(bool value); +bool check(int value); int direct_if() { int value; @@ -74,7 +74,7 @@ int logical_or() { int nested_comparison() { int value; - if ((value = get_value() < 0) == 1) // $ Alert // BAD + if ((value = get_value() < 0) == false) // $ Alert // BAD return value; return 0; } @@ -173,7 +173,7 @@ int compound_assignment_without_comparison() { int switch_expression() { int value; - switch (value = get_value() < 0) { // GOOD: This query only covers branching conditions. + switch (value = get_value() < 0) { // GOOD: The switch operand is not used as a truth value. case 0: return value; default: @@ -277,14 +277,87 @@ int overloaded_comparison() { int assignment_as_call_argument() { int value; - if (identity(value = get_value() < 0)) // $ Alert // BAD: The ambiguous syntax is still in a condition. + if (check(value = get_value() < 0)) // GOOD: The assignment only computes an integer argument. 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. + if ((value = get_value() < 0, get_other_value())) // GOOD: The assignment result is discarded. + return value; + return 0; +} + +int truth_valued_assignment_in_comma_expression() { + int value; + if ((get_other_value(), value = get_value() < 0)) // $ Alert // BAD + return value; + return 0; +} + +int comma_assignment_under_comparison() { + int value; + if ((get_other_value(), value = get_value() < 0) == false) // $ Alert // BAD + return value; + return 0; +} + +int nested_truth_valued_comparisons() { + int value; + if (((value = get_value() < 0) == false) == false) // $ Alert // BAD + return value; + return 0; +} + +int truth_valued_conditional_then_arm(bool flag) { + int value; + if (flag ? (value = get_value() < 0) : false) // $ Alert // BAD + return value; + return 0; +} + +int truth_valued_conditional_else_arm(bool flag) { + int value; + if (flag ? false : (value = get_value() < 0)) // $ Alert // BAD + return value; + return 0; +} + +int nested_truth_valued_comma_expression() { + int value; + if ((get_other_value(), (get_other_value(), value = get_value() < 0))) // $ Alert // BAD + return value; + return 0; +} + +int nested_discarded_comma_expression() { + int value; + if (((get_other_value(), value = get_value() < 0), get_other_value())) // GOOD: Discarded. + return value; + return 0; +} + +int nested_comma_assignment_under_comparison() { + int value; + if ((get_other_value(), (get_other_value(), value = get_value() < 0)) == false) // $ Alert // BAD + return value; + return 0; +} + +bool logical_value_outside_branch() { + int value; + return (value = get_value() < 0) && get_other_value(); // $ Alert // BAD +} + +bool logical_not_outside_branch() { + int value; + return !(value = get_value() < 0); // $ Alert // BAD +} + +int truth_valued_assignment_inside_call_argument() { + int value; + if (check((value = get_value() < 0) && get_other_value())) // $ Alert // BAD return value; return 0; }