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

Set up CodeQL scans and fixed several findings #1601

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

artem-smotrakov
Copy link
Contributor

I'd like to suggest setting CodeQL scans for CuraEngine. CodeQL is a static-analysis engine that can help with detecting security and other issues. It can be easily run in a GitHub workflow. The suggested config runs scans on PRs and the main branch. Findings are going to be posted as comments in pull requests. Or, they can also be found in the Security tab.

Here is an example workflow run for CodeQL.

CodeQL reported several findings, mostly multiplication overflows and wrong format strings. I've looked into them and tried to fix them but this definitely needs a review because I have not writtten C code for a long time :) If this PR is accepted, the baseline is going to be 0 findings.

Several findings have been reported in stb_image.h that is pulled while building CuraEngine. I've updated stb_image.h to the latest commit and added a patch that addresses findings at build time. This can be removed once these issues are fixed in the upstream repository.

Updated format strings to use correct argument types for numbers.
- Scan files only in src
- Skip tests in CodeQL scans
- Updated stb version
- Patched potential multiplication overflows in stb_image.h
@rburema
Copy link
Member

rburema commented Mar 24, 2022

Hi! Thanks for bringing this up. The general idea of a static analysis has been floated several times, and I appreciate that you've taken the time and effort to actually do something!

While I don't have the time right now to dive into them (we've reduced the time we work on PR's until 5.0 is out), I do have some remarks already:

I do have several issues with this: It seems that CodeQL cannot be run as part of CI/CD unless you're a paying customer:

... These Terms do not authorize, and the Software may not be used for any purpose not expressly set forth above, including: ... To otherwise or in any other context generate any CodeQL database for or during automated analysis, CI or CD, whether as part of normal engineering processes or another context. ...

Less importantly: A similar restriction is applied to closed source. Now Cura and CuraEngine will never be closed of course, but Ultimaker in general does have closed source software (mostly maintained by other teams, but still:) We'd prefer a workflow that could be applied to multiple projects across multiple teams (of course we already have tons of exceptions to that, but that isn't a reason to not be critical).

That doesn't mean this is out of the question of course, but something to think about. (Also in general: Why use CodeQL over another static analyser, why should we choose that one? Maybe there are very good reasons, but none of those are supplied in the PR text.)

That leaves us with the fixes themselves (like I said, no deep dive right now):

  • These days, casting new style is preferred (static_cast<double>(x) instead of the old implicit (double) x -- this is useful in case you change x from a primitive value to a pointer, since the old style will happily reinterpret-cast instead).
  • We'd prefer it if the code would use our typedef's if applicable. A coord_t is a coord_t for a reason after all!

@artem-smotrakov
Copy link
Contributor Author

Hi @rburema As far as I know, CodeQL is free for open-source projects regardless whether it is used in CI/CD or manually. I have not compared CodeQL to similar tools. I think, and it is my personal opinion, this is one of the best tools available for open-source projects. It's extendable, can be run in CI/CD naturally via GitHub Actions, rules/queries are open-source as well. I'll let you decide whether you'd like to use it 👍

@jellespijker jellespijker self-requested a review November 11, 2022 08:56
@jellespijker jellespijker self-assigned this Nov 11, 2022
@jellespijker jellespijker added the PR: Community Contribution 👑 Community Contribution PR's label Nov 30, 2022
@jellespijker jellespijker added the PR: Automation and build 🤖 Improving CI/CT/CD workflows label Feb 16, 2024
@jellespijker jellespijker removed their request for review March 15, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Automation and build 🤖 Improving CI/CT/CD workflows PR: Community Contribution 👑 Community Contribution PR's
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants