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

[theinfosecguy]

Adds cpp/ambiguous-assignment-of-comparison to flag conditions 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]

This isn't the only good variant. The following is good too, because it shows that the developer really meant it.

  if ((status = (read_status() < 0))) // GOOD: explicitly assigns the comparison result.
    return status;

[geoffw0]

That's true, but we don't need to show every possibility in the .qhelp example. The test is the place to be thorough.

[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!

[geoffw0]

I don't think the double brackets are communicating anything to the reader here.

Suggested change

	if ((status = read_status() < 0)) // BAD: assigns the comparison result.
	if (status = read_status() < 0) // BAD: assigns the comparison result.

[geoffw0]

What's the motivating case here? I think there's a good argument we want this for things like the right argument of a comma expression, but I'm not convinced we care about the arguments to function calls that just happens to be located somewhere inside an if expression.

[geoffw0]

We can simplify this a little:

Suggested change

	result = any(WhileStmt s).getCondition()
	or
	result = any(DoStmt s).getCondition()
	or
	result = any(ForStmt s).getCondition()
	result = any(Loop s).getCondition()

[geoffw0]

I have an AI suggestion, which is to relax this function from expressions that "control branching" to expressions "whose value is used as a truth value", adding these cases:

Suggested change

	}
	or
	result = any(UnaryLogicalOperation op).getAnOperand()
	or
	result = any(BinaryLogicalOperation op).getAnOperand()
	}

Looking at your code again, the absence of these two cases might be the main reason you found yourself needing occursInCondition. With them, we might be better off without occursInCondition at all???