Skip to content

Commit

Permalink
Modify rule S1265: Add support for sized delete (M23_358) and globall…
Browse files Browse the repository at this point in the history
…y improve the rule (CPP-5138 CPP-1313 )
  • Loading branch information
loic-joly-sonarsource authored May 31, 2024
1 parent 8da8a3e commit 4f26fad
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 10 deletions.
3 changes: 2 additions & 1 deletion rules/S1265/cfamily/metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
},
"tags": [
"cppcoreguidelines",
"cert"
"cert",
"based-on-misra"
],
"extra": {
"replacementRules": [
Expand Down
63 changes: 54 additions & 9 deletions rules/S1265/cfamily/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,26 +1,32 @@
== Why is this an issue?

The `operator new` allocates memory for objects, and the `operator delete` frees the memory allocated by the matching `operator new`. When a class needs to customize memory allocation, it can override the `operator new` to use a custom memory allocation strategy and override the `operator delete` accordingly.
The `operator new` allocates memory for objects, and the `operator delete` frees the memory allocated by the matching `operator new`. It is possible to customize these operations, either for a specific class (by overloading these operators in the class) or globally (by defining them in the global namespace, they will replace the default version).

If the `operator delete` is not overridden alongside the `operator new`, the program will call its default implementation, which may not be suitable for the custom memory allocation strategy used by the overridden `operator new`.
If the `operator delete` is not overloaded alongside the `operator new`, the program will call its default implementation, which may not be suitable for the custom memory allocation strategy used by the overloaded `operator new`.

For instance, if the `operator new` draws memory from a preallocated buffer instead of allocating memory, the `operator delete` should not call the `free` function to release the memory. Reciprocally, if the `operator new` allocate memory with `malloc`, the `operator delete` must call `free`.

On the other hand, if the `operator delete` is overridden without overriding the `operator new`, it is suspicious because it may not correctly release the memory allocated by the default `operator new`.
On the other hand, if the `operator delete` is overloaded without overloading the `operator new`, it is suspicious because it may not correctly release the memory allocated by the default `operator new`.

By defining the `operator delete` along with the `operator new`, the memory is deallocated in a way consistent with the custom allocation strategy used by the `operator new`.

Up to this point, we mentioned `operator new` and `operator delete`, but it is a simplification. There are many different forms of https://en.cppreference.com/w/cpp/memory/new/operator_new[`operator new`] and https://en.cppreference.com/w/cpp/memory/new/operator_delete[`operator delete`] (for a single object or an array, with an extra alignment parameter... see the links for a full list), and the consistency between new and delete should be enforced for each form.

For instance, if `++void * operator new[]( std::size_t count, std::align_val_t al );++` is defined (for arrays, with extra alignment), then `++void operator delete[]( void* ptr, std::align_val_t al ) noexcept;++` should be defined too.

Additionally, since {cpp}17, it is possible to define a version of the delete operator with an additional size argument, alongside the unsized version of `operator delete`. When overloading these operators in a class, defining both a sized and an unsized version of operator delete is useless, since the unsized version will always be preferred. However, for free replacement, it is necessary to specify both versions since the language does not specify which version will be called.

=== What is the potential impact?

Overriding only one of the two operators may result in your program consuming more and more memory over time, eventually leading to a crash.
Overloading only one of the two operators may result in your program consuming more and more memory over time, eventually leading to a crash.

Deallocating memory that was not allocated with the corresponding strategy results in undefined behavior.

== How to fix it

Each override of the `operator new` should have a matching overridden `operator delete` and vice versa.
Each overload of the `operator new` should have a matching overload `operator delete` and vice versa. Since {cpp}17, within a class, define only a sized or an unsized version of `operator delete`, but as a free function, define both.

=== Code examples
=== Example with overloaded operators in a class

Imagine a custom allocator `MyAllocator`:

Expand Down Expand Up @@ -48,7 +54,7 @@ public:
----
class MyClass {
public:
// Noncompliant: there is no matching override of the delete operator
// Noncompliant: there is no matching overload of the delete operator
void* operator new(size_t size) {
return allocator.allocate(size);
}
Expand All @@ -60,7 +66,7 @@ private:
void f() {
MyClass* obj = new MyClass();
delete obj; // It will call the default implementation of the operator delete
// and this latter will not call the MyAllocator::deallocate function to correctly release the memory
// which will not call the MyAllocator::deallocate function to correctly release the memory
}
----

Expand All @@ -74,7 +80,7 @@ public:
return allocator.allocate(size);
}
void operator delete(void* p) {
void operator delete(void* p) noexcept {
allocator.deallocate(p);
}
Expand All @@ -88,6 +94,43 @@ void f() {
}
----

=== Example with replacement of global allocators

In the following example, the intent is to replace allocation functions with system-specific variants.

==== Noncompliant code example

[source,cpp,diff-id=2,diff-type=noncompliant]
----
// Noncompliant: The sized version of delete is not replaced
void* operator new(std::size_t count) {
return SystemSpecificNew(count);
}
void operator delete(void* ptr ) noexcept {
return SystemSpecificDelete(ptr);
}
----
==== Compliant solution

[source,cpp,diff-id=2,diff-type=compliant]
----
void* operator new(std::size_t count) {
return SystemSpecificNew(count);
}
void operator delete(void* ptr) noexcept {
return SystemSpecificDelete(ptr);
}
// Compliant: Even if it does exactly the same as the unsized version, this sized
// version of delete replaces the default-provided one that probably deallocates
// memory in a different and incompatible way.
void operator delete(void* ptr, std::size_t) noexcept {
return SystemSpecificDelete(ptr);
}
----
== Resources

=== Documentation
Expand All @@ -102,6 +145,8 @@ void f() {
=== External coding guidelines

* {cpp} Core Guidelines - https://github.com/isocpp/CppCoreGuidelines/blob/e49158a/CppCoreGuidelines.md#r15-always-overload-matched-allocationdeallocation-pairs[R.15: Always overload matched allocation/deallocation pairs]
* MISRA {cpp}:2023, 21.6.4 - If a project defines either a sized or unsized version of a global
operator delete, then both shall be defined

=== Related rules

Expand Down

0 comments on commit 4f26fad

Please sign in to comment.