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
| File | Description |
|---|---|
| rules.csv | Re-points RULE-21-6-1 to the new Banned7 package. |
| rule_packages/cpp/Banned7.json | Adds package metadata for RULE-21-6-1. |
| cpp/misra/src/rules/RULE-21-6-1/DynamicMemoryShouldNotBeUsed.ql | Implements the new RULE-21-6-1 query and alert messaging. |
| cpp/misra/src/rules/RULE-21-6-2/DynamicMemoryManagedManually.ql | Refactors to reuse shared dynamic-memory modeling from a common library. |
| cpp/common/src/codingstandards/cpp/DynamicMemory.qll | Introduces shared modeling for placement-new operators and std allocator allocate/deallocate members. |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/Banned7.qll | Adds the autogenerated exclusions/metadata wrapper for the new package query. |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll | Registers Banned7 so exclusions/metadata plumbing recognizes it. |
| cpp/misra/test/rules/RULE-21-6-1/test.cpp | Adds unit tests covering many direct/indirect allocation/deallocation scenarios. |
| cpp/misra/test/rules/RULE-21-6-1/DynamicMemoryShouldNotBeUsed.qlref | Links the test to the production RULE-21-6-1 query. |
| cpp/misra/test/rules/RULE-21-6-1/DynamicMemoryShouldNotBeUsed.expected | Adds expected results for RULE-21-6-1 test cases. |
| cpp/common/test/includes/standard-library/any.h | Adds stub declarations for std::any used by tests. |
| cpp/common/test/includes/standard-library/any | Adds wrapper include for <any>. |
| cpp/common/test/includes/standard-library/bitset.h | Adds stub declarations for std::bitset used by tests. |
| cpp/common/test/includes/standard-library/bitset | Adds wrapper include for <bitset>. |
| cpp/common/test/includes/standard-library/deque.h | Adjusts std::deque stub constructors/includes used by tests. |
| cpp/common/test/includes/standard-library/filesystem.h | Adds stub declarations for std::filesystem::path used by tests. |
| cpp/common/test/includes/standard-library/filesystem | Adds wrapper include for <filesystem>. |
| cpp/common/test/includes/standard-library/forward_list.h | Adds stub declarations for std::forward_list used by tests. |
| cpp/common/test/includes/standard-library/forward_list | Adds wrapper include for <forward_list>. |
| cpp/common/test/includes/standard-library/fstream.h | Expands stream stubs/aliases used by tests. |
| cpp/common/test/includes/standard-library/fstream | Adds wrapper include for <fstream>. |
| cpp/common/test/includes/standard-library/functional.h | Extends reference_wrapper stub to support test usage. |
| cpp/common/test/includes/standard-library/future.h | Adds stubs for std::future/promise/async used by tests. |
| cpp/common/test/includes/standard-library/future | Adds wrapper include for <future>. |
| cpp/common/test/includes/standard-library/istream.h | Minor include-guard formatting change. |
| cpp/common/test/includes/standard-library/list.h | Adds stub declarations for std::list used by tests. |
| cpp/common/test/includes/standard-library/list | Adds wrapper include for <list>. |
| cpp/common/test/includes/standard-library/map | Adds constructors/initializer_list support to map/multimap stubs for tests. |
| cpp/common/test/includes/standard-library/memory.h | Adjusts smart pointer/allocator-related stubs used by tests. |
| cpp/common/test/includes/standard-library/optional | Minor include-guard formatting change. |
| cpp/common/test/includes/standard-library/queue.h | Adds stub declarations for std::queue/priority_queue used by tests. |
| cpp/common/test/includes/standard-library/queue | Adds wrapper include for <queue>. |
| cpp/common/test/includes/standard-library/regex.h | Adds stub declarations for std::regex used by tests. |
| cpp/common/test/includes/standard-library/regex | Adds wrapper include for <regex>. |
| cpp/common/test/includes/standard-library/set | Adds constructors/initializer_list support to set/multiset stubs for tests. |
| cpp/common/test/includes/standard-library/sstream.h | Expands stringstream/istringstream/ostringstream stubs and aliases for tests. |
| cpp/common/test/includes/standard-library/stack.h | Adds stub declarations for std::stack used by tests. |
| cpp/common/test/includes/standard-library/stack | Adds wrapper include for <stack>. |
| cpp/common/test/includes/standard-library/string | Adds wstring/u16string/u32string aliases for tests. |
| cpp/common/test/includes/standard-library/tuple.h | Expands tuple stub constructors/templates used by tests. |
| cpp/common/test/includes/standard-library/unordered_map.h | Adds unordered_(multi)map stubs used by tests. |
| cpp/common/test/includes/standard-library/unordered_map | Adds wrapper include for <unordered_map>. |
| cpp/common/test/includes/standard-library/unordered_set.h | Adds unordered_(multi)set stubs used by tests. |
| cpp/common/test/includes/standard-library/unordered_set | Adds wrapper include for <unordered_set>. |
| cpp/common/test/includes/standard-library/utility.h | Expands pair constructors/templates used by tests. |
| cpp/common/test/includes/standard-library/valarray.h | Adds std::valarray stub used by tests. |
| cpp/common/test/includes/standard-library/valarray | Adds wrapper include for <valarray>. |
| cpp/common/test/includes/standard-library/variant.h | Adds std::variant stub used by tests. |
| cpp/common/test/includes/standard-library/variant | Adds wrapper include for <variant>. |
Copilot's findings
- Files reviewed: 46/48 changed files
- Comments generated: 1
| template<typename F, typename... Args> | ||
| auto async(F&&, Args&&...) -> future<decltype(declval<F>()(declval<Args>()...))>; | ||
|
|
||
| template<typename F, typename... Args> | ||
| auto async(launch, F&&, Args&&...) -> future<decltype(declval<F>()(declval<Args>()...))>; |
There was a problem hiding this comment.
future.h uses declval in the async return type, but the header does not include <utility> (where std::declval is declared in these stubs) nor provide its own forward declaration. This can make the stub header ill-formed when included before <utility> (as in the new RULE-21-6-1 test). Add #include <utility> or declare declval before it is used.
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.
| * The operator functions have a `std::size_t` as their first parameter and a | ||
| * `void*` parameter somewhere in the rest of the parameter list. | ||
| */ | ||
| class PlacementNewOrNewArrayAllocationFunction extends AllocationFunction { |
There was a problem hiding this comment.
Can we combine this with cpp/common/src/allocations/CustomOperatorNewDelete ?
| this instanceof AllocationFunction and | ||
| not this instanceof PlacementNewOrNewArrayAllocationFunction | ||
| or | ||
| this.hasGlobalOrStdName("aligned_alloc") |
There was a problem hiding this comment.
Can you add a comment for why aligned_alloc has to be special cased here, or, can you add aligned_alloc to AllocationFunction?
| * Includes `vector`, `deque`, `list`, `forward_list`, `set`, `map`, `multiset`, `multimap`, | ||
| * `unordered_set`, `unordered_map`, `unordered_multiset`, `unordered_multimap`, and `valarray`. | ||
| */ | ||
| class AllocatorContainerConstructor extends IndirectDynamicMemoryAllocatingFunction { |
There was a problem hiding this comment.
I think we need to implement this via a very precise and subtle reading of the specified hueristic in the standard:
any instantiation of a C++ standard library entity having a template argument that is a specialization of
std::allocatoris a violation of this rule
This is actually important because:
// Non compliant, because it uses std::allocator<int> which is a specialization of std::allocator
std::vector<int, std::allocator<int>> non_compliant_vec1;
// Non compliant, because the 2nd type parameter is defaulted to std::allocator<T>
// so std::vector<int> and std::vector<int, std::allocator<int>> are the same type.
std::vector<int> non_compliant_vec2;
// Some setup code that uses static storage instead of the heap
char buf[128];
struct nonallocating {
void *allocate(int size) { return buf; }
void deallocate(void *p) { }
};
// Compliant: this vector does _not_ use a specialization of std::allocator<T> !
std::vector<int, nonallocating> my_nonallocating_vec;
The above pattern is I believe pretty reasonable and/or common in the embedded space.
Many of the cases you found here are awesome and will actually still be necessary. For instance, vector and string only use dynamic memory if their allocator uses dynamic memory, but std::ifstream always allocates and is non compliant by the 2nd part of the rule's hueristic: ... as is any call to a C++ standard library function that may use dynamic memory.
Happy to talk more and clarify later.
| this.getAParameter().getUnderlyingType() instanceof VoidPointerType | ||
| } | ||
| } | ||
| import codingstandards.cpp.DynamicMemory |
There was a problem hiding this comment.
Nice code reuse!
| #include <utility> | ||
| #include <valarray> | ||
| #include <variant> | ||
| #include <vector> |
There was a problem hiding this comment.
excellent test coverage as well!
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.