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

C++: Fix FPs to cpp/return-stack-allocated-memory #18309

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

Conversation

calumgrant
Copy link
Contributor

@calumgrant calumgrant commented Dec 17, 2024

Pull Request checklist

All query authors

Internal query authors only

  • Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to .ql, .qll, or .qhelp files. See the documentation (internal access required).
  • Changes are validated at scale (internal access required).
  • Adding a new query? Consider also adding the query to autofix.

@github-actions github-actions bot added the C++ label Dec 17, 2024
@calumgrant
Copy link
Contributor Author

DCA results show no regressions in manual build, and expected FPs removed in BMN.

@calumgrant calumgrant force-pushed the calumgrant/bmn/return-stack-allocated-memory branch from 3ae30c2 to fabaceb Compare December 18, 2024 14:37
@calumgrant calumgrant marked this pull request as ready for review December 18, 2024 14:38
@Copilot Copilot bot review requested due to automatic review settings December 18, 2024 14:38
@calumgrant calumgrant requested a review from a team as a code owner December 18, 2024 14:38

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 3 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql: Language not supported
  • cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp: Language not supported

Tip: Turn on automatic Copilot reviews for this repository to get quick feedback on every pull request. Learn more

Comment on lines +253 to +256
UNKNOWN_TYPE x;
return x; // GOOD: Don't report error types
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't recall what we did in other cases, but it might be better to have all test cases with parse errors in separate files, because that will mean we catch parse errors that are introduced by accident in "regular" tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not a bad idea to start doing that.

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.

Two remarks.

---
category: minorAnalysis
---
* The "Returning stack-allocated memory" query (`cpp/return-stack-allocated-memory`) no longer produces results if there is an extraction error in the type of the function.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not accurate. It no longer produces a result when there's an ErroneousType anywhere on the potential must-flow path (but that is too much detail for a change note). So it might be that the return type of the function is known, but that some critical type within the function is not.

Something along the following lines (modifying the example from the QL file):

   int* test() {
      UNKNOWN_TYPE s;
      return &s.x; // BAD: &s.x is an address of a variable on the stack.
   }

Comment on lines +253 to +256
UNKNOWN_TYPE x;
return x; // GOOD: Don't report error types
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this example is actually fine, because no pointer to x is being returned.

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.

2 participants