LGTM.com - false positive Assignment where comparison was intended

[dscho]

"Assignment where comparison was intended" suggests to surround assignment with parentheses, but that's already the case.

I actually run this in a GitHub workflow. The code in question is here: https://github.com/dscho/git/blob/2ab72e89eeff833e248b77256cfa2f59893f3442/ident.c#L175-L175

This rule kicks in and warns about it: https://github.com/github/codeql/blob/main/cpp/ql/src/Likely%20Bugs/Likely%20Typos/AssignWhereCompareMeant.ql

The recommendation suggests that "it may be better to surround it with parentheses.", but that is clearly already the case.

cc: @MathiasVP

[MathiasVP]

Hi @dscho,

Thanks for reporting this!

I think I may have given you bad advice earlier. It turns out that the query is actually working as intended, and that the problem is really in the qhelp for the query. The qhelp (correctly) tells you to parenthesize the expression to mark this assignment as intentional. It doesn't, however, tell you that you should add an additional set of parentheses other than those you already have.

So I think this change should remove the alert from your codebase:

- } else if ((email = query_user_email()) && email[0]) {
+ } else if (((email = query_user_email())) && email[0]) {

The motivation for this additional set of parentheses is that (email = query_user_email()) && email[0]) changes the precedence (since = binds more loosely than &&), so it's not really a reliable way to mark this assignment as intentional. Where's an expression like ((email = query_user_email())) && email[0]) actually contains a redundant set of parentheses, which we take as an indication that the assignment is intentional.

I hope that helps!

I'll create an issue to make the qhelp for this file more precise.

[dscho]

I fear the chances are very slim that the git/git project will follow your reasoning. How can I disable this rule when using security-and-quality?

[MathiasVP]

I fear the chances are very slim that the git/git project will follow your reasoning. How can I disable this rule when using security-and-quality?

You can do the following:

• Create a new suite (see here)
• Import the existing security-and-quality suite
• Exclude the query you don’t want to run

Here's an example for a different project:

- import: codeql-suites/java-code-scanning.qls
  from: codeql-java
- exclude:
    id:
    - java/spring-disabled-csrf-protection

[dscho]

I fear the chances are very slim that the git/git project will follow your reasoning.

I would like to expand on that because there is a bug in the rule: the assignment is enclosed in extra parentheses, so what you are asking is to enclose it in double parentheses.

This is different from literally every other static analyzer I've used: GCC, Clang, Coverity.

In short: it really looks to me as if the rule, in its current form, should be improved to match common expectations.

[MathiasVP]

Thanks for the additional information. We acknowledge that the behavior of the rule should be improved, and we're currently discussing what that improvement should be :)

[dscho]

Thanks for the additional information. We acknowledge that the behavior of the rule should be improved, and we're currently discussing what that improvement should be :)

Any news on this, @MathiasVP ?

[MathiasVP]

Hi @dscho,

I've opened a PR with a fix here: #5604. Hopefully, that should also cover the false positive you've noticed.

[dscho]

Thank you, @MathiasVP. Is there any convenient way how I could test this before the PR is merged?

[MathiasVP]

Thank you, @MathiasVP. Is there any convenient way how I could test this before the PR is merged?

No problem! 😄
I'm not sure exactly how convenient it is, but maybe you can use a custom configuration file in your workflow that points to the branch in #5604. See https://docs.github.com/en/code-security/secure-coding/configuring-code-scanning#example-configuration-files for an example of this.

I would recommend just doing a local checkout of #5604.

[criemen]

Our internal testing shows

=== Changes per project (+new -fixed) ===

g/git/git                                                                   +0 -1

=== Changes per query (+new -fixed) ===

cpp/assign-where-compare-meant                                              +0 -1

Presumably this is the reported false positive.