Skip to content

C++: Nullness.qll bug fixes #9785

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 8 commits into from
Jul 12, 2022
Merged

C++: Nullness.qll bug fixes #9785

merged 8 commits into from
Jul 12, 2022

Conversation

bdrodes
Copy link
Contributor

@bdrodes bdrodes commented Jul 6, 2022

Corrects a bug in nullCheckExpr and validCheckExpr in Nullness.qll for logical AND expressions. Previously only checking null/valid on expressions in the right hand side of the AND operation. Now checking both left and right.

@bdrodes bdrodes requested a review from a team as a code owner July 6, 2022 19:22
@github-actions github-actions bot added the C++ label Jul 6, 2022
@jketema
Copy link
Contributor

jketema commented Jul 6, 2022

Thanks for this. This looks correct to me. However, a test which exercises the new behavior would be useful. I can point you in the right direction for this. However, for that it would be good to know if you found this while running one of our queries, and, if so, which one.

@bdrodes
Copy link
Contributor Author

bdrodes commented Jul 6, 2022

@jketema I had tested this with examples like:

char *d = (char*)malloc(10);
if(true && d)
//...
if(d && true)
// ...

And variants (!d). Then posing the query if the expression is a valid/null check.

If you point me to the right spot I'm sure I can put in something that will illustrate the bug and fix.

To answer your question though, I found this bug making my own query.

@jketema
Copy link
Contributor

jketema commented Jul 6, 2022

To answer your question though, I found this bug making my own query.

No worries. If there had been a specific query you were looking at it would have been easiest to add a test to that query. I don't think we test this functionality directly. Let me think a bit about how to best solve this.

@bdrodes
Copy link
Contributor Author

bdrodes commented Jul 7, 2022

@jketema Sounds good. I was probably going to make 2 more PRs related to Nullness.ql today. Would it be easier to bundle them into this PR or to do them separately?

@bdrodes bdrodes changed the title Fixing logic bug with LogicalAndExpr Nullness.qll bug fixes Jul 7, 2022
@jketema
Copy link
Contributor

jketema commented Jul 7, 2022

Would it be easier to bundle them into this PR or to do them separately?

Depends a bit on their size. If they're small, I don't have a problem with having them as part of the current PR.

@bdrodes
Copy link
Contributor Author

bdrodes commented Jul 7, 2022

Would it be easier to bundle them into this PR or to do them separately?

Depends a bit on their size. If they're small, I don't have a problem with having them as part of the current PR.

I ended up only having one addition change which is now part of this PR. It's very small.

@jketema jketema changed the title Nullness.qll bug fixes C++: Nullness.qll bug fixes Jul 7, 2022
@jketema
Copy link
Contributor

jketema commented Jul 11, 2022

I've created some tests for Nullness.qll: #9793. The first commit of this PR should trigger two changes in those tests.

It was not immediately clear to me what you second commit is supposed to achieve, so it's probably not yet covered.

@bdrodes
Copy link
Contributor Author

bdrodes commented Jul 11, 2022

@jketema The second commit was added to address situations where there is an initialization in a conditional guard. e.g.,
if(char* c = (char*)malloc(10)) ...

In this case the initialization is a validation check, but wasn't being detected as a validation check previously.

@jketema
Copy link
Contributor

jketema commented Jul 11, 2022

Thanks for clarifying.

#9793 has been merged. Could you rebase this PR and update the tests/test results as appropriate? If you're not sure how to do the tests, please just rebase, and I'll give you a hand.

@bdrodes
Copy link
Contributor Author

bdrodes commented Jul 11, 2022

@jketema Yea I could use a hand to properly update the test results.

@jketema
Copy link
Contributor

jketema commented Jul 11, 2022

Yea I could use a hand to properly update the test results.

In that case, please rebase this PR on top of the latest main and I'll give you a hand.

@bdrodes
Copy link
Contributor Author

bdrodes commented Jul 11, 2022

Is a merge of main sufficient or do I need to do I need to do anything fancier?

@jketema
Copy link
Contributor

jketema commented Jul 11, 2022

Is a merge of main sufficient or do I need to do I need to do anything fancier?

That works too 😄

@jketema jketema self-assigned this Jul 11, 2022
@jketema
Copy link
Contributor

jketema commented Jul 12, 2022

I've updated the tests and did some other minor fixes.

@jketema
Copy link
Contributor

jketema commented Jul 12, 2022

@geoffw0 @rdmarsh2 Could one of you review this? Since I now made some changes myself here. I'll also start one of our internal experiments, as this is used under the hood in some of our queries.

| test.cpp:21:9:21:14 | ... = ... | test.cpp:5:13:5:13 | v | is null | is not valid |
| test.cpp:21:9:21:14 | ... = ... | test.cpp:7:10:7:10 | b | is not null | is valid |
| test.cpp:22:17:22:17 | b | test.cpp:7:10:7:10 | b | is not null | is valid |
| test.cpp:22:9:22:14 | ... = ... | test.cpp:5:13:5:13 | v | is not null | is not valid |
| test.cpp:22:9:22:14 | ... = ... | test.cpp:7:13:7:13 | c | is not null | is not valid |
Copy link
Contributor

Choose a reason for hiding this comment

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

Please would you explain these two results? It looks to me like if c = !v is true, c will be true (only after the assignment) and v must be false.

If this is just a limitation of the analysis, I'm happy with that and happy for it to be documented by this test. I just want to check I'm understanding correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, those are a bit weird. There's two things going on here:

  1. RHSs of assignments are not covered, as they're technically not part of a condition. So, those expressions for each variable will always result in is not null and is not valid
  2. The c case is a C++17-style if-initializer (note the c in the condition on the next line of the expected file). These also fall outside the condition that is evaluated in the if, and hence we also get is not null and is not valid there.

Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
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.

Looks good to me. 👍 👍

@jketema jketema merged commit c18428f into github:main Jul 12, 2022
@jketema
Copy link
Contributor

jketema commented Jul 12, 2022

@bdrodes Merged! Thanks for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants