Modify rule S2637: Expand and adjust for LaYC

## Review A dedicated reviewer checked the rule description successfully for: - [x] logical errors and incorrect information - [x] information gaps and missing content - [x] text style and tone - [x] PR summary and labels follow [the guidelines](https://github.com/SonarSource/rspec/#to-modify-an-existing-rule) --------- Co-authored-by: Arseniy Zaostrovnykh <[email protected]>
SonarSource · Oct 4, 2023 · f45132d · f45132d
1 parent 03b6321
commit f45132d
Show file tree

Hide file tree

Showing 7 changed files with 209 additions and 28 deletions.
diff --git a/rules/S2259/cfamily/rule.adoc b/rules/S2259/cfamily/rule.adoc
@@ -107,21 +107,7 @@ if (p2 != NULL) {
 
 === Going the extra mile
 
-In {cpp}, it is preferred to use reference parameters (`Type const&` or `Type&`), or pass objects by value (`Type`),
-instead of a pointer (`Type const*` or `Type*`), if the argument is expected to never be null.
-
-[source,cpp]
-----
-// Precondition: graph != null
-bool isDAG(Graph const* graph);
----- 
-
-The preferred signature would be:
-
-[source,cpp]
-----
-bool isDAG(Graph const& graph);
----- 
+include::../../../shared_content/cfamily/reference_over_nonnull_pointer.adoc[]
 
 
 == Resources
@@ -132,6 +118,12 @@ bool isDAG(Graph const& graph);
 * CERT - https://wiki.sei.cmu.edu/confluence/x/aDdGBQ[EXP01-J.Do not use a null in a case where an object is required]
 * CWE - https://cwe.mitre.org/data/definitions/476[476 NULL Pointer Dereference]
 
+=== External coding guidelines
+
+* {cpp} Core Guidelines - https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md[F.16: For "in" parameters, pass cheaply-copied types by value and others by reference to const]
+* {cpp} Core Guidelines - https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md[F.17: For "in-out" parameters, pass by reference to non-const]
+
+
 === Related rules
 
 * S3807 detects calls to C library functions that require valid, non-null pointers with null pointer arguments

diff --git a/rules/S2637/cfamily/rule.adoc b/rules/S2637/cfamily/rule.adoc
@@ -1,20 +1,191 @@
+Pointers marked as "nonnull" may not be set to null, since they are typically not null-checked before use.
+
 == Why is this an issue?
 
-Functions return values and parameters values marked ``++nonnull++`` are assumed to have non-null values and are not typically null-checked before use. Therefore setting one of these values to ``++null++``, could cause null pointer dereferences at runtime.
+A function's return value and parameters may be decorated with attributes to convey additional information to the compiler and/or other developers.
+
+A commonly used attribute is `nonnull` which can be used to mark a function's return value and parameters as shown in the following:
+
+[source,cpp]
+----
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+__attribute__((returns_nonnull)) int *
+make_array_copy(__attribute__((nonnull)) int *src, size_t len) {
+  int *dst = (int *)malloc(len * sizeof(int));
+  if (dst == NULL) {
+    perror("malloc failed");
+    exit(1);
+  }
+  memcpy(dst, src, len);
+  return dst;
+}
+----
+
+The `nonnull` attribute is meant for other developers and as a hint for compilers.
+Values marked as `nonnull` are assumed to have non-null values.
+
+However, developers may accidentally break the `nonnull` attribute as shown in the following code snippet:
+
+[source,cpp]
+----
+__attribute__((returns_nonnull))
+int* foo(__attribute__((nonnull)) int* x) {
+  x = 0; // Noncompliant: `x` is marked "nonnull" but is set to null
+  foo(0); // Noncompliant: null is passed as an argument marked as "nonnull"
+  return 0; // Noncompliant: return value is marked "nonnull" but null is returned
+}
+----
+
+Failing to adhere to the attribute may introduce serious program errors.
+In particular, the compiler does not enforce that values marked as `nonnull` are indeed non-null at runtime; it is the developers' responsibility to adhere to the attribute.
+These values are typically _not_ null-checked before use.
+Setting a value marked as `nonnull` to null (i.e., `NULL`, `0` or `nullptr`) is hence likely to cause a null-pointer dereference.
+Compilers may even apply optimizations based on this attribute and might, for instance, _remove_ an explicit null-check if the parameter is declared as `nonnull` -- even in code outside of the function with the attribute.
+
+Note that the `nonnull` attribute is a GNU extension (see https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-nonnull-function-attribute[nonnull] and https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-returns_005fnonnull-function-attribute[returns_nonnull]) which many compiler vendors have implemented.
+
+
+== What is the potential impact?
 
-=== Noncompliant code example
+In case a program dereferences a null pointer, it's behavior is undefined.
+For programs that exercise undefined behavior the compiler no longer needs to adhere to the language standard and the program has no meaning assigned to it.
+
+In practice, dereferencing a null pointer may lead to program crashes, or the application may appear to execute correctly while losing data or producing incorrect results.
+
+Besides affecting the application's availability, null-pointer dereferences may lead to malicious code execution, in rare circumstances.
+If null is equivalent to the 0x0 memory address that can be accessed by privileged code, writing, and reading memory is possible, which compromises the integrity and confidentiality of the application.
+
+Because compilers may apply optimizations based on the `nonnull` attribute, not respecting `nonnull` can also introduce more complex bugs such as resource leaks or infinite loops as indicated in the following code snippet:
 
 [source,cpp]
 ----
+struct Node {
+  int data;
+  Node *next; // NULL for a tail node.
+};
+
+size_t len(__attribute__((nonnull)) Node *n) {
+  size_t l = 0;
+  while (n) {
+    ++l;
+    n = n->next;
+  }
+  return l;
+}
+----
+
+
+== How to fix it
+
+Ensure not to pass null values when non-null arguments are expected, do not return a null value when a non-null return value is expected, and do not assign null to parameters marked as non-null.
+This especially holds for library functions, which frequently require `nonnull` pointer parameters.
+
+On other occasions, it might be more appropriate to remove the attribute.
+
+
+=== Code examples
+
+==== Noncompliant code example
+
+[source,cpp,diff-id=1,diff-type=noncompliant]
+----
+__attribute__((returns_nonnull))
+int* foo(__attribute__((nonnull)) int* x) {
+  *x = 42;
+  return x;
+}
+
+void bar() {
+  int *p = nullptr;
+  int *q = foo(p); // Noncompliant: null value is passed as an argument marked "nonnull"
+}
+----
+
+==== Compliant solution
+
+[source,cpp,diff-id=1,diff-type=compliant]
+----
+__attribute__((returns_nonnull))
+int* foo(__attribute__((nonnull)) int* x) {
+  *x = 42;
+  return x;
+}
+
+void bar() {
+  int i = 0;
+  int *p = &i;
+  int *q = foo(p); // Compliant: `p` points to a valid memory location
+}
+----
+
+==== Noncompliant code example
+
+[source,cpp,diff-id=2,diff-type=noncompliant]
+----
+__attribute__((returns_nonnull))
+int* foo() {
+  return nullptr; // Noncompliant: function may not return a null pointer
+}
+----
+
+==== Compliant solution
+
+[source,cpp,diff-id=2,diff-type=compliant]
+----
 __attribute__((returns_nonnull))
-int* nonnull(__attribute__((nonnull)) int* parameter) {
-  parameter = 0; // Noncompliant - "parameter" is marked "nonnull" but is set to null.
-  nonnull(0); // Noncompliant - Parameter "parameter" to this call is marked "nonnull" but null is passed.
-  return 0; // Noncompliant - This function's return value is marked "nonnull" but null is returned.
+int* foo() {
+  int *p = new int(0);
+  return p; // Compliant: `p` points to a valid memory location
+}
+----
+
+==== Noncompliant code example
+
+[source,cpp,diff-id=3,diff-type=noncompliant]
+----
+void process(int *p);
+
+void foo(__attribute__((nonnull)) int *p) {
+  p = nullptr; // Noncompliant: `p` is marked "nonnull" but is set to null
+  process(p);
+}
+----
+
+==== Compliant solution
+
+[source,cpp,diff-id=3,diff-type=compliant]
+----
+void process(int *p);
+
+void foo(__attribute__((nonnull)) int *p) {
+  process(p);
 }
 ----
 
-include::../see.adoc[]
+
+=== Going the extra mile
+
+include::../../../shared_content/cfamily/reference_over_nonnull_pointer.adoc[]
+
+
+== Resources
+
+include::../standards.adoc[]
+
+=== External coding guidelines
+
+* {cpp} Core Guidelines - https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md[F.16: For "in" parameters, pass cheaply-copied types by value and others by reference to const]
+* {cpp} Core Guidelines - https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md[F.17: For "in-out" parameters, pass by reference to non-const]
+
+
+=== Related rules
+
+* S2259 detects null-pointer dereferences
+* S3807 detects calls to C library functions that require valid, non-null pointers with null pointer arguments
+
 
 ifdef::env-github,rspecator-view[]
 

diff --git a/rules/S2637/java/rule.adoc b/rules/S2637/java/rule.adoc
@@ -31,7 +31,9 @@ public class MainClass {
 ----
 
 
-include::../see.adoc[]
+== Resources
+
+include::../standards.adoc[]
 
 ifdef::env-github,rspecator-view[]
 

diff --git a/rules/S2637/metadata.json b/rules/S2637/metadata.json
@@ -33,11 +33,12 @@
       476
     ],
     "CERT": [
+      "EXP34-C.",
       "EXP01-J."
     ]
   },
   "defaultQualityProfiles": [
     "Sonar way"
   ],
-  "quickfix": "unknown"
+  "quickfix": "infeasible"
 }
diff --git a/rules/S2637/see.adoc b/rules/S2637/see.adoc
diff --git a/rules/S2637/standards.adoc b/rules/S2637/standards.adoc
@@ -0,0 +1,5 @@
+=== Standards
+
+* CERT - https://wiki.sei.cmu.edu/confluence/x/QdcxBQ[EXP34-C. Do not dereference null pointers]
+* CERT - https://wiki.sei.cmu.edu/confluence/display/java/EXP01-J.+Do+not+use+a+null+in+a+case+where+an+object+is+required[EXP01-J. Do not use a null in a case where an object is required]
+* CWE - https://cwe.mitre.org/data/definitions/476[476 NULL Pointer Dereference]
diff --git a/shared_content/cfamily/reference_over_nonnull_pointer.adoc b/shared_content/cfamily/reference_over_nonnull_pointer.adoc
@@ -0,0 +1,14 @@
+In {cpp}, it is preferred to use reference parameters (`Type const&` or `Type&`), or pass objects by value (`Type`), instead of a pointer (`Type const*` or `Type*`), if the argument is expected to never be null.
+
+[source,cpp]
+----
+// Precondition: graph != null
+bool isDAG(Graph const* graph);
+---- 
+
+The preferred signature would be:
+
+[source,cpp]
+----
+bool isDAG(Graph const& graph);
+----