Enable GitHub static analysis

[yadij]

[kinkie]

i like this. It would be even better if the commit comment contained links or other explanations about what checks are done. It would be also nice to know what features are analyzed and what are the libraries included in the ubuntu-latest image on GitHub, so to be more clear about what code paths are tickled.

These are asks, not mandatory. In any case, the more checks the better

[yadij]

I'm not exactly sure about any of the details for this. Approval to use the scanner on Squid sources came through today and what you can see from the "Checks" tab are the logs and artifacts from the scan.

From the build log (Checks -> Analyse -> "..." menu -> Raw Logs) it appears that none of the optional libraries are present. This is just a basic Ubuntu with build chain installed.

[kinkie]

It would be good if we could add the extra things we need :/

On Wed, 22 Jul 2020 at 13:56, Amos Jeffries [email protected] wrote:

I'm not exactly sure about any of the details for this. Approval to use the scanner on Squid sources came through today and what you can see from the "Checks" tab are the logs and artifacts from the scan.

From the build log (Checks -> Analyse -> "..." menu -> Raw Logs) it appears that none of the optional libraries are present. This is just a basic Ubuntu with build chain installed.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/squid-cache/squid/pull/693#issuecomment-662411138, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHPVDBDOIRLVTDWXBLUBFLR43HXPANCNFSM4PEJQOIA .

-- @mobile

[rousskov]

Aha, I now realized that this was a WIP PR. That clarifies why it was waiting for its author. The premature approval confused me. I have adjusted PR title/description (this is not just about PRs) and converted the PR to Draft PR.

[yadij]

Warning: Code scanning cannot determine the alerts introduced or fixed by this pull request, because 1 analysis was not found.

Would it be possible to fix the above before this goes in?

AFAIK, no. We have to merge the addition of this config file to master to get the piece it is complaining about.

It would be nice to fix/address the actual alerts (in a separate PR), so that we can easily know when a PR actually introduces a new problem, but I do not insist on that, of course.

If I am understanding you that is the idea of this. Provided the QL analysis proves to be low in false-positives we make this one of Anubis requirements and don't let in code with warnings.

I am not sure why this PR was marked as waiting for the author long time ago.

Records says "@rousskov added the S-waiting-for-author label on Dec 12, 2020 "

For my part, it was waiting on consensus that we wanted to use this tool. Then got stalled behind other work.

[EDIT: I should add. Due to the Ubuntu system checked on not having a lot of optional libraries Squid uses reliability of the results is questionable. Many parts of the code are simply unable to be checked.

[rousskov]

It would be nice to fix/address the actual alerts (in a separate PR), so that we can easily know when a PR actually introduces a new problem, but I do not insist on that, of course.

If I am understanding you that is the idea of this. Provided the QL analysis proves to be low in false-positives we make this one of Anubis requirements and don't let in code with warnings.

I meant fixing alerts in a separate PR that is merged before this PR. This (unmerged) PR is enough to expose all the alerts AFAICT. If we merge this PR first, then, for every subsequent PR, we will waste time trying to understand which alerts are worth paying attention to. Yes, it is possible to distinguish old alerts from new ones, but it would be much easier/clearer if there were no old alerts to start with.

Perhaps even more importantly, that "fixing" PR would also demonstrate that these alerts are actually useful, can be addressed, can be disabled if needed, etc. Right now, I would have to spend a lot of time developing that knowledge and, technically, that knowledge is required to properly review this PR. I would rather review the fixing PR(s) instead!

I am not sure why this PR was marked as waiting for the author long time ago.

Records says "@rousskov added the S-waiting-for-author label on Dec 12, 2020 "

That known record does not answer the "why" question I was asking. I found the answer; see https://github.com/squid-cache/squid/pull/693#issuecomment-897204462

Due to the Ubuntu system checked on not having a lot of optional libraries Squid uses reliability of the results is questionable. Many parts of the code are simply unable to be checked.

IMO, that fact makes the results incomplete rather than questionable. Incomplete alerts can still be very useful, of course. Can we configure the analysis to add the missing libraries? I think that is also what Francesco was asking in https://github.com/squid-cache/squid/pull/693#issuecomment-662412942.

[yadij]

The analyzer is now working for basic builds. We just need the found issues to be resolved before this can merge.

[rousskov]

I adjusted the PR title a little to be more specific. Please edit further as needed.

[github-advanced-security[bot]]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.