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

Create codeql-analysis.yml #21

Merged
merged 1 commit into from
Mar 5, 2021
Merged

Create codeql-analysis.yml #21

merged 1 commit into from
Mar 5, 2021

Conversation

danlangford
Copy link
Collaborator

im interested to see what sort of things this analysis finds

@danlangford
Copy link
Collaborator Author

@pappde
Copy link
Owner

pappde commented Mar 5, 2021

This argument should be of type 'unsigned int' but is of type 'unsigned long'

The issue here is that "m_properties" is a U32 (correct) but U32 is defined as "unsigned long" - it should be "unsigned int" (long is 64 bits).

@pappde
Copy link
Owner

pappde commented Mar 5, 2021

Some comments

  1. I'm surprised it didn't find this one that VS reports:
		if (d->HasProperty(BME_PROPERTY_OPTION))
		{
			if (_move.m_option_die[i]>1)
				return false;
		}

src\bmai.cpp(1506): warning C4804: '>': unsafe use of type 'bool' in operation

Here, m_option_die returns a "bool" which generally when implicitly casting to an integer gives you 0 or 1, so the "return false" will never run. (NOTE: this particular issue is covered in #18.)

Is there perhaps a setting for CodeQL to make it check additional types of code issues?

  1. In the GitHub actions output, it doesn't have a checkmark for CodeQL. It says:

1 analysis not found

Is it actually running? I found the following explanation, but not sure what it means.

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

@danlangford
Copy link
Collaborator Author

danlangford commented Mar 5, 2021

If you merge this PR it will more regularly run on all your branches and PRs.

I don’t know if it will find more and I don’t know how to add more checks but the run I saw was sort of a trial run on this PR not a real run that sores the analytics in an easy to view place… not until you merge the PR

@pappde pappde merged commit a0f4659 into main Mar 5, 2021
@pappde pappde deleted the codeql-analysis branch March 5, 2021 21:01
@pappde
Copy link
Owner

pappde commented Mar 5, 2021

Merged PR. We'll see if the "analysis not found" message goes away with subsequent PR.

@danlangford
Copy link
Collaborator Author

Now the alerts are very easy to see in the Security tab if this repo. Now to see if we can add more checks

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