Skip to content

C++: Add another CWE-825 query #8173

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

Merged
merged 12 commits into from
Feb 24, 2022

Conversation

MathiasVP
Copy link
Contributor

This is a new IR-based query that can hopefully serve as a high-precision version of cpp/stack-address-escape. Instead of raising an alert on every assignment that writes the address of a local variable in a non-local variable, the query only raises an alert if we find a subsequent load of that global variable after the local variable's function has returned.

The query doesn't find a lot of results currently. But all the results that it does find look like true positives to me.

@MathiasVP MathiasVP force-pushed the add-using-expired-stack-address-query branch from 7f2d4e4 to ea35f56 Compare February 22, 2022 19:12
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of the non-query part. I'll look at the query separately.

@jketema
Copy link
Contributor

jketema commented Feb 23, 2022

Someone with more knowledge of CodeQL than me probably also needs to have a look at this.

MathiasVP and others added 4 commits February 23, 2022 09:33
…ess.ql

Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com>
…ess.ql

Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com>
…ess.cpp

Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com>
…ess.qhelp

Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com>
@MathiasVP
Copy link
Contributor Author

Review of the non-query part. I'll look at the query separately.

Thanks a lot for all the comments. I think my e key is starting to wear off >.<

geoffw0
geoffw0 previously approved these changes Feb 23, 2022
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks promising, I have only nitpicks.

I appreciate thorough tests!

…ess.ql

Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
@MathiasVP
Copy link
Contributor Author

I've been super unlucky with the last two DCA runs:

  • The first one failed because there was a null-pointer issue in the CLI's structured logging. This has been fixed so I restarted DCA with a more recent internal repo SHA.
  • So now I accidentally included the structured binding stuff from the internal repo without pulling in the changes to the dbscheme from C++: Add table that identifies C++ structured bindings #7928 🤦. So the DCA run failed because of a dbscheme mismatch.

The third time's the charm (I hope!) 🤞

jketema
jketema previously approved these changes Feb 24, 2022
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming third time is a charm when it comes to DCA runs.

@MathiasVP MathiasVP added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Feb 24, 2022
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also happy with this PR now (DCA pending...)

Copy link
Contributor

@hubwriter hubwriter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from a docs PoV. 👍

@MathiasVP
Copy link
Contributor Author

DCA looks good 🎉.

@MathiasVP MathiasVP merged commit ab3cad7 into github:main Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ documentation ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants