C/C++: Detect ambiguous assignment of comparison results

[theinfosecguy]

Adds cpp/ambiguous-assignment-of-comparison to flag ambiguous assignments when the assignment result is used as a truth value, such as:

if ((status = read_status() < 0))

The query distinguishes this from explicitly grouped assign-then-compare and compare-then-assign expressions. It includes C and C++ tests, query help, and query-suite integration.

Local targeted and neighboring tests pass. The motivating regression is detected, and a run against git/git produced no alerts.

Fixes #22286

[ryao]

While I am happy you did the work for me to get this into a PR and made what appear to be improvements, would you add an Original-patch-by: to the commit message to credit my prior work?

Also, have you run your variant against any major corpora (e.g. Linux, curl, OpenZFS) to verify the lack of FPs in production code, like I did with the original version? If it helps:

https://docs.github.com/en/code-security/how-tos/find-and-fix-code-vulnerabilities/scan-from-the-command-line/download-databases

[ryao]

While this is normally intentional, I believe not isExplicitlyGrouped(comparison) precludes this.

[geoffw0]

Why is that? It seems like the types of the expressions and the bracketing are mostly independent concerns.

[geoffw0]

I've reviewed the code and docs from a technical perspective (I haven't looked through all the test cases yet). This looks quite promising. I've also done a mass (MRVA) run and found quite a high rate of true positive results!

[github-actions]

QHelp previews:

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

Ambiguous assignment of comparison used as truth value

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.

Recommendation

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.

Example

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.

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;
}

References

• SEI CERT C Coding Standard: EXP00-C. Use parentheses for precedence of operation.
• C++ reference: Operator precedence.
• Common Weakness Enumeration: CWE-783.

[geoffw0]

Nice thorough tests. I've nit-picked a couple of cases but they're not important ones, and you might have more insight into them than I have, from knowing the cases that motivated this query.

---

I've gained a lot of confidence in this query from reviewing it - just a few small things to discuss and decide if we want to make any changes or not. 👍

[geoffw0]

Thank you for making the changes. The CodeQL looks good to me, as do the tests and MRVA (bulk testing) results. I think I said it before, but I'm seeing lots of true positive results in real world data. :)

I've requested a quick review of the .qhelp from the docs team, we should be able to merge soon after that.