Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CodeQL GitHub code scanning workflow #2076

Merged
merged 5 commits into from
Feb 18, 2022

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Feb 8, 2022

Adds a GitHub code scanning workflow running CodeQL, see GitHub documentation "About code scanning with CodeQL".

The workflow is currently configured to run all queries in the "security-and-quality" suite; this is more than the default which only runs certain security queries. I have fixed some of the findings; it looks like all other ones are false positives, or can be ignored.

It appears by default generated and test code is scanned as well (and cannot be excluded?), but results for it can be filtered in the security view.
Edit: Have changed the build command to mvn compile; therefore test code is not analyzed anymore.

I have marked this pull request as draft for now to get some feedback. No worries in case you don't want to include this.

@Marcono1234 Marcono1234 marked this pull request as draft February 8, 2022 22:29
`annotations.proto` also seems to be only relevant for tests because the test
explicitly registers them as extensions. By default the Proto adapter does not
consider them.
@Marcono1234 Marcono1234 force-pushed the marcono1234/code-scanning branch from 2b2d8d4 to a29ad1e Compare February 12, 2022 23:42
@Marcono1234 Marcono1234 force-pushed the marcono1234/code-scanning branch from f51f169 to 7337057 Compare February 16, 2022 20:01
@Marcono1234 Marcono1234 marked this pull request as ready for review February 16, 2022 20:05
@eamonnmcmanus
Copy link
Member

Were you planning to fix the remaining CodeQL failures?

@Marcono1234
Copy link
Collaborator Author

I think all remaining findings are false positives, or can be ignored (for now):

  • "Array index out of bounds" for JavaWriter.java
    This is part of the codegen module, which is non-functional (codegen project is incomplete / non-functional #2007); additionally as mentioned by you on that issue, that class is most likely a third party class coming from JavaPoet
  • "Container contents are never accessed" for CollectionsDeserializationBenchmark.java
    For this benchmark accessing the collection elements is not necessary, assuming that the Gson unit tests would detect any incorrect functional Gson behavior. I assume the List is used in this benchmark to make it more realistic?
  • "Dereferenced variable may be null" for UtcDateTypeAdapter.java and ISO8601Utils.java
    These findings seem to be reported due to the date == null check below the try statement. However, to me that looks reasonable; in general these methods do not seem to support null as date, but validation of the other arguments might have caused an exception before any null check. Additionally in both cases Gson always seems to call the methods with non-null arguments. (And these classes originally come from Jackson, as indicated by the comments)
  • "Dereferenced variable may be null" for LinkedTreeMap.java
    I am not very familiar with the code of that class, but to me it looks like the dereferencing expressions are guarded by the check on the value of the delta variable, and that only seems to be -2 respectively 2 when rightHeight respectively leftHeight are > 0, which won't be the case when right or left are null.
  • "Reference equality test on strings" for LazilyParsedNumber.java
    Check happens in equals and is followed by equals call for the String value. Maybe using == here is premature optimization, but it is functionally correct.
  • "Missing catch of NumberFormatException"
    Maybe some of them could be revisited in the future, one of them is JsonReader throws NumberFormatException which does not contain location info #1564.
  • ... all other notes can probably be ignored (for now).

You as maintainer can dismiss the false positive alerts later on, if you like.

@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Feb 18, 2022

Thanks, I've dismissed the three remaining errors. I think this will be helpful!

@eamonnmcmanus eamonnmcmanus merged commit 49ddab9 into google:master Feb 18, 2022
@Marcono1234 Marcono1234 deleted the marcono1234/code-scanning branch February 18, 2022 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants