Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new MISRA C++:2023 rule package (Banned7) and implements RULE-21-6-1 (“Dynamic memory should not be used”) as a new CodeQL query, with accompanying tests and supporting library/stub updates.
Changes:
- Introduces
Banned7rule package metadata and wires RULE-21-6-1 to it inrules.csv. - Adds the new RULE-21-6-1 query and a comprehensive unit test (+
.expected/.qlref). - Factors shared dynamic-memory modeling helpers into
codingstandards.cpp.DynamicMemoryand updates RULE-21-6-2 to use it; expands C++ standard-library stubs to support the new test.
Show a summary per file
Copilot's findings
- Files reviewed: 46/48 changed files
- Comments generated: 1
MichaelRFairhurst
left a comment
MichaelRFairhurst
left a comment
There was a problem hiding this comment.
Let's explore how complex it may be to implement the std::allocator based logic.
If it's really complex to implement, I think it's fair to say that we don't support non-allocating specializations of allocating types in this rule. In that case, users can simply file the extra deviations when they do that kind of stuff, and we can lower the precision and add it to the implementation_scope.
MichaelRFairhurst
left a comment
MichaelRFairhurst
left a comment
There was a problem hiding this comment.
Looks great! Two suggestions if you want to implement them.
There was a problem hiding this comment.
Consider not reporting destructors.
Reading the rule text, it first says "any instantiation of a C++ Standard Library entity having a template argument that is a specialization of std::allocator is a violation of this rule." I think if the intent was to flag destructors, they would have said "any instantiation or deletion of ..."
Reading more of the rule text, we can definitely justify flagging destructors. For example, after talking about instantiations, the rule says, "as is any call to a C++ Standard Library function that may use dynamic memory." Which could be interpreted to mean destructors, for sure. It also talks about "free" specifically, and how that can occur implicitly.
So there's definitely a case to be made for reporting ~vector(). And we should definitely definitely flag delete vec for some std::vector<T> *vec.
That said, I'm not sure RAII implicit deletions will be useful to report. If a user deviates this rule to declare std::vector<T> vec; then it's implied that they're OK with that destructor being called. It's also slightly weird because there's no explicit location in the code to deviate.
There was a problem hiding this comment.
I believe there's a middle ground to be taken here. The call detected above is obviously hidden away from the user because it's the RAII at action and not a manual invocation from the user. I am sure that the rule only applies to the latter and obviously not the former.
The case for reporting such RAII destructors being called can be countered by the argument that report on these calls very likely pollute the results, and these are implied by a more fundamental cause that is the use of std::vector that allocates.
So, a manual invocation of the destructor is what we want to report here. I am confident that calls to destructors, although not mentioned directly, count as dynamic memory management considering the intention of the rule.
All in all, the manual destructor calls are what we want to report along the same lines as we report calls to free and operator delete.
There was a problem hiding this comment.
Seems reasonable to me.
Basically, the two cases left over to consider are:
vec->~vector(); // explicit call
// and
delete vec;
I'm not sure the first one is common enough to be worth tracking and reporting, but we could definitely track it and flag it. And luckily enough, the 2nd one will already be reported.
There was a problem hiding this comment.
Rolled back in 0e85f55 and added justification.
Description
Add rule package
Banned7that contains Rule 21.6.1.Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
RULE-21-6-1RULE-21-6-2Release change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
🚨🚨🚨
Reviewer: Confirm that format of shared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
Query development review checklist
For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:
Author
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Reviewer
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.