C++: Fix FPs to cpp/return-stack-allocated-memory #18309
Conversation
|
DCA results show no regressions in manual build, and expected FPs removed in BMN. |
calumgrant
force-pushed
the
calumgrant/bmn/return-stack-allocated-memory
branch
from
3ae30c2 to
fabaceb
Compare
There was a problem hiding this comment.
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
| UNKNOWN_TYPE x; | ||
| return x; // GOOD: Don't report error types | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's not a bad idea to start doing that.
| --- | ||
| 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. |
There was a problem hiding this comment.
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.
}| UNKNOWN_TYPE x; | ||
| return x; // GOOD: Don't report error types | ||
| } |
There was a problem hiding this comment.
I think this example is actually fine, because no pointer to x is being returned.
There was a problem hiding this comment.
There was a regression (embarrassing result), where this was actually flagged as an alert. But the commit history does not contain the failing test being fixed.
cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp
Outdated
Show resolved
Hide resolved
|
|
||
| void* test_error_pointer() { | ||
| UNKNOWN_TYPE x; | ||
| return &x; // GOOD: Don't know what &x means |
There was a problem hiding this comment.
This is "BAD" (I guess unless & is actually an overloaded operator, because who knows in that case), because we're taking the address of a local variable.
I'm fine with silencing these, but the comment needs to be updated.
There was a problem hiding this comment.
I agree, they are "probably" bad. Do we even need a comment?
| return &x; // GOOD: Don't know what &x means | |
| return &x; // BAD [FALSE NEGATIVE]: &x is an error |
vs.
| return &x; // GOOD: Don't know what &x means | |
| return &x; // BAD [FALSE NEGATIVE] |
There was a problem hiding this comment.
Just // BAD [FALSE NEGATIVE] is sufficient in both cases, I think.
|
|
||
| int* test_error_pointer_member() { | ||
| UNKNOWN_TYPE x; | ||
| return &x.y; // GOOD: Don't know what x.y means |
…ackAllocatedMemory/test.cpp Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com>
Pull Request checklist
All query authors
.qhelp. See the documentation in this repository.Internal query authors only
.ql,.qll, or.qhelpfiles. See the documentation (internal access required).