Merge pull request #332 from github/lcartey/rule-11-4-improvements

Rule 11.4 improvements

c/misra/src/rules/RULE-11-4/ConversionBetweenPointerToObjectAndIntegerType.ql

@@ -13,14 +13,73 @@
 
 import cpp
 import codingstandards.c.misra
+import codingstandards.cpp.Macro
 import codingstandards.cpp.Pointers
 
-from CStyleCast cast, Type typeFrom, Type typeTo
+MacroInvocation getAMacroInvocation(CStyleCast cast) { result.getAnExpandedElement() = cast }
+
+Macro getPrimaryMacro(CStyleCast cast) {
+  exists(MacroInvocation mi |
+    mi = getAMacroInvocation(cast) and
+    not exists(MacroInvocation otherMi |
+      otherMi = getAMacroInvocation(cast) and otherMi.getParentInvocation() = mi
+    ) and
+    result = mi.getMacro()
+  )
+}
+
+Macro getNonFunctionPrimaryMacro(CStyleCast cast) {
+  result = getPrimaryMacro(cast) and
+  not result instanceof FunctionLikeMacro
+}
+
+from
+  Locatable primaryLocation, CStyleCast cast, Type typeFrom, Type typeTo, string message,
+  string extraMessage, Locatable optionalPlaceholderLocation, string optionalPlaceholderMessage
 where
   not isExcluded(cast, Pointers1Package::conversionBetweenPointerToObjectAndIntegerTypeQuery()) and
   typeFrom = cast.getExpr().getUnderlyingType() and
   typeTo = cast.getUnderlyingType() and
-  [typeFrom, typeTo] instanceof IntegralType and
-  [typeFrom, typeTo] instanceof PointerToObjectType and
-  not isNullPointerConstant(cast.getExpr())
-select cast, "Cast performed between a pointer to object type and a pointer to an integer type."
+  (
+    typeFrom instanceof PointerToObjectType and
+    typeTo instanceof IntegralType and
+    message =
+      "Cast from pointer to object type '" + typeFrom + "' to integer type '" + typeTo + "'" +
+        extraMessage + "."
+    or
+    typeFrom instanceof IntegralType and
+    typeTo instanceof PointerToObjectType and
+    message =
+      "Cast from integer type '" + typeFrom + "' to pointer to object type '" + typeTo + "'" +
+        extraMessage + "."
+  ) and
+  not isNullPointerConstant(cast.getExpr()) and
+  // If this alert is arising through a non-function-like macro expansion, flag the macro instead, to
+  // help make the alerts more manageable. We only do this for non-function-like macros because they
+  // cannot be context specific.
+  if exists(getNonFunctionPrimaryMacro(cast))
+  then
+    primaryLocation = getNonFunctionPrimaryMacro(cast) and
+    extraMessage = "" and
+    optionalPlaceholderLocation = primaryLocation and
+    optionalPlaceholderMessage = ""
+  else (
+    primaryLocation = cast and
+    // If the cast is in a macro expansion which is context specific, we still report the original
+    // location, but also add a link to the most specific macro that contains the cast, to aid
+    // validation.
+    if exists(getPrimaryMacro(cast))
+    then
+      extraMessage = " from expansion of macro $@" and
+      exists(Macro m |
+        m = getPrimaryMacro(cast) and
+        optionalPlaceholderLocation = m and
+        optionalPlaceholderMessage = m.getName()
+      )
+    else (
+      extraMessage = "" and
+      optionalPlaceholderLocation = cast and
+      optionalPlaceholderMessage = ""
+    )
+  )
+select primaryLocation, message, optionalPlaceholderLocation, optionalPlaceholderMessage

c/misra/src/rules/RULE-11-6/CastBetweenPointerToVoidAndArithmeticType.ql

@@ -22,5 +22,5 @@ where
   typeTo = cast.getUnderlyingType() and
   [typeFrom, typeTo] instanceof ArithmeticType and
   [typeFrom, typeTo] instanceof VoidPointerType and
-  not isNullPointerConstant(cast.getExpr())
+  not cast.getExpr() instanceof Zero
 select cast, "Cast performed between a pointer to void type and an arithmetic type."

c/misra/src/rules/RULE-11-9/MacroNullNotUsedAsIntegerNullPointerConstant.ql

@@ -18,15 +18,25 @@ import codingstandards.cpp.Type
 from Zero zero, Expr e, string type
 where
   not isExcluded(zero, Pointers1Package::macroNullNotUsedAsIntegerNullPointerConstantQuery()) and
-  // exclude the base-case (NULL macros and void pointer casts)
-  not isNullPointerConstant(zero) and
+  // Exclude the base-case (NULL macros and void pointer casts)
+  // Note: we cannot use the isNullPointerConstant predicate here because it permits
+  //       the use of `0` without casting, which is prohibited here.
+  not (
+    zero.findRootCause() instanceof NullMacro
+    or
+    // integer constant `0` explicitly cast to void pointer
+    exists(Conversion c | c = zero.getConversion() |
+      not c.isImplicit() and
+      c.getUnderlyingType() instanceof VoidPointerType
+    )
+  ) and
   (
     // ?: operator
     exists(ConditionalExpr parent |
       (
-        parent.getThen().getAChild*() = zero and parent.getElse().getType() instanceof PointerType
+        parent.getThen() = zero and parent.getElse().getType() instanceof PointerType
         or
-        parent.getElse().getAChild*() = zero and parent.getThen().getType() instanceof PointerType
+        parent.getElse() = zero and parent.getThen().getType() instanceof PointerType
       ) and
       // exclude a common conditional pattern used in macros such as 'assert'
       not parent.isInMacroExpansion() and

c/misra/test/rules/RULE-11-1/ConversionBetweenFunctionPointerAndOtherType.expected

@@ -1,7 +1,6 @@
 | test.c:11:8:11:16 | (fp1 *)... | Cast performed between a function pointer and another type. |
 | test.c:11:8:11:16 | (fp1)... | Cast performed between a function pointer and another type. |
 | test.c:12:14:12:23 | (void *)... | Cast performed between a function pointer and another type. |
-| test.c:14:8:14:15 | (fp2)... | Cast performed between a function pointer and another type. |
 | test.c:15:8:15:15 | (fp2)... | Cast performed between a function pointer and another type. |
 | test.c:22:12:22:13 | (fp1)... | Cast performed between a function pointer and another type. |
 | test.c:25:8:25:9 | (fp1)... | Cast performed between a function pointer and another type. |

c/misra/test/rules/RULE-11-1/test.c

@@ -11,7 +11,7 @@ void f1(void) {
   v1 = (fp1 *)v2;        // NON_COMPLIANT
   void *v3 = (void *)v1; // NON_COMPLIANT
 
-  v2 = (fp2 *)0; // NON_COMPLIANT
+  v2 = (fp2 *)0; // COMPLIANT - null pointer constant
   v2 = (fp2 *)1; // NON_COMPLIANT
 
   pfp2 v4;

c/misra/test/rules/RULE-11-4/ConversionBetweenPointerToObjectAndIntegerType.expected

@@ -1,6 +1,6 @@
-| test.c:5:21:5:42 | (unsigned int)... | Cast performed between a pointer to object type and a pointer to an integer type. |
-| test.c:5:35:5:42 | (int *)... | Cast performed between a pointer to object type and a pointer to an integer type. |
-| test.c:6:21:6:37 | (unsigned int)... | Cast performed between a pointer to object type and a pointer to an integer type. |
-| test.c:8:8:8:24 | (unsigned int)... | Cast performed between a pointer to object type and a pointer to an integer type. |
-| test.c:10:22:10:22 | (unsigned int *)... | Cast performed between a pointer to object type and a pointer to an integer type. |
-| test.c:12:22:12:39 | (unsigned int *)... | Cast performed between a pointer to object type and a pointer to an integer type. |
+| test.c:6:21:6:37 | (unsigned int)... | Cast from pointer to object type 'unsigned int *' to integer type 'unsigned int'. | test.c:6:21:6:37 | (unsigned int)... |  |
+| test.c:8:8:8:24 | (unsigned int)... | Cast from pointer to object type 'unsigned int *' to integer type 'unsigned int'. | test.c:8:8:8:24 | (unsigned int)... |  |
+| test.c:12:22:12:39 | (unsigned int *)... | Cast from integer type 'unsigned int' to pointer to object type 'unsigned int *'. | test.c:12:22:12:39 | (unsigned int *)... |  |
+| test.c:15:1:15:24 | #define FOO (int *)0x200 | Cast from integer type 'int' to pointer to object type 'int *'. | test.c:15:1:15:24 | #define FOO (int *)0x200 |  |
+| test.c:23:3:23:22 | (int *)... | Cast from integer type 'int' to pointer to object type 'int *' from expansion of macro $@. | test.c:17:1:17:34 | #define FOO_FUNCTIONAL(x) (int *)x | FOO_FUNCTIONAL |
+| test.c:24:14:24:25 | (int *)... | Cast from integer type 'int' to pointer to object type 'int *' from expansion of macro $@. | test.c:18:1:18:23 | #define FOO_INSERT(x) x | FOO_INSERT |

c/misra/test/rules/RULE-11-4/test.c

@@ -2,12 +2,24 @@
 
 void f1(void) {
   unsigned int v1 = (unsigned int)(void *)0; // COMPLIANT
-  unsigned int v2 = (unsigned int)(int *)0;  // NON_COMPLIANT
+  unsigned int v2 = (unsigned int)(int *)0;  // COMPLIANT
   unsigned int v3 = (unsigned int)&v2;       // NON_COMPLIANT
   v3 = v2;                                   // COMPLIANT
   v3 = (unsigned int)&v2;                    // NON_COMPLIANT
   v3 = NULL;                                 // COMPLIANT
-  unsigned int *v4 = 0;                      // NON_COMPLIANT
+  unsigned int *v4 = 0;                      // COMPLIANT
   unsigned int *v5 = NULL;                   // COMPLIANT
   unsigned int *v6 = (unsigned int *)v2;     // NON_COMPLIANT
+}
+
+#define FOO (int *)0x200 // NON_COMPLIANT
+#define FOO_WRAPPER FOO;
+#define FOO_FUNCTIONAL(x) (int *)x
+#define FOO_INSERT(x) x
+
+void test_macros() {
+  FOO;                      // Issue is reported at the macro
+  FOO_WRAPPER;              // Issue is reported at the macro
+  FOO_FUNCTIONAL(0x200);    // NON_COMPLIANT
+  FOO_INSERT((int *)0x200); // NON_COMPLIANT
 }

c/misra/test/rules/RULE-11-9/MacroNullNotUsedAsIntegerNullPointerConstant.expected

@@ -1,4 +1,5 @@
 | test.c:15:13:15:13 | 0 | $@ uses zero-value integer constant expression as null pointer constant. | test.c:15:7:15:13 | ... == ... | Equality operator |
 | test.c:17:8:17:8 | 0 | $@ uses zero-value integer constant expression as null pointer constant. | test.c:17:3:17:8 | ... = ... | Assignment to pointer |
-| test.c:25:20:25:20 | 0 | $@ uses zero-value integer constant expression as null pointer constant. | test.c:25:3:25:35 | ... ? ... : ... | Ternary operator |
-| test.c:25:20:25:20 | 0 | $@ uses zero-value integer constant expression as null pointer constant. | test.c:25:15:25:20 | ... = ... | Assignment to pointer |
+| test.c:23:13:23:13 | 0 | $@ uses zero-value integer constant expression as null pointer constant. | test.c:23:3:23:13 | ... ? ... : ... | Ternary operator |
+| test.c:24:8:24:8 | 0 | $@ uses zero-value integer constant expression as null pointer constant. | test.c:24:3:24:13 | ... ? ... : ... | Ternary operator |
+| test.c:31:14:31:14 | 0 | $@ uses zero-value integer constant expression as null pointer constant. | test.c:31:9:31:14 | ... = ... | Assignment to pointer |

c/misra/test/rules/RULE-11-9/test.c

@@ -19,9 +19,16 @@ void *f1(void *p1, int p2) {
   p1 = NULL;      // COMPLIANT
   if (p2 == 0) {  // COMPLIANT
     return NULL;
-  }                                     // COMPLIANT
-  (p1) ? (p1 = NULL) : (p1 = NULL);     // COMPLIANT
-  (p2 > 0) ? (p1 = NULL) : (p1 = NULL); // COMPLIANT
-  (p2 > 0) ? (p1 = 0) : (p1 = NULL);    // NON_COMPLIANT
-  return 0;                             // COMPLIANT
+  }
+  p2 ? p1 : 0;         // NON_COMPLIANT
+  p2 ? 0 : p1;         // NON_COMPLIANT
+  p2 ? (void *)0 : p1; // COMPLIANT
+  p2 ? p1 : (void *)0; // COMPLIANT
+  p2 ? p2 : 0;         // COMPLIANT - p2 is not a pointer type
+  p2 ? 0 : p2;         // COMPLIANT - p2 is not a pointer type
+  int x;
+  int *y;
+  p2 ? (p1 = 0) : p1; // NON_COMPLIANT - p1 is a pointer type
+  p2 ? (p2 = 0) : p1; // COMPLIANT - p2 is not a pointer type
+  return 0;           // COMPLIANT
 }

change_notes/2023-07-28-rule-11-4-improvements.md

@@ -0,0 +1,12 @@
+ - `RULE-11-1` - `ConversionBetweenFunctionPointerAndOtherType.ql`:
+    - Fixed issue #331 - consider `0` a null pointer constant.
+ - `RULE-11-4` - `ConversionBetweenPointerToObjectAndIntegerType.ql`:
+   - Fixed issue #331 - consider `0` a null pointer constant. 
+   - Improve reporting of the order of the cast and the actual types involved.
+   - Improve reporting where the result is expanded from a macro by either reporting the macro itself (if it is not dependent on the context) or by including a link to the macro in the alert message.
+ - `RULE-11-5` - `ConversionFromPointerToVoidIntoPointerToObject.ql`:
+    - Fixed issue #331 - consider `0` a null pointer constant.
+ - `RULE-11-6` - `CastBetweenPointerToVoidAndArithmeticType.ql`:
+    - Fixed issue #331 - accept integer constant expressions with value `0` instead of null pointer constants.
+ - `RULE-11-9` - `MacroNullNotUsedAsIntegerNullPointerConstant.ql`:
+    - Remove false positives in branches of ternary expressions, where `0` was used correctly.

cpp/common/src/codingstandards/cpp/Pointers.qll

@@ -62,12 +62,9 @@ class ArrayPointerArithmeticExpr extends PointerArithmeticExpr, ArrayExpr {
 predicate isNullPointerConstant(Expr e) {
   e.findRootCause() instanceof NullMacro
   or
-  exists(CStyleCast c |
-    not c.isImplicit() and
-    c.getExpr() = e and
-    e instanceof Zero and
-    c.getType() instanceof VoidPointerType
-  )
+  // 8.11 Pointer type conversions states:
+  // A null pointer constant, i.e. the value 0, optionally cast to void *.
+  e instanceof Zero
   or
   isNullPointerConstant(e.(Conversion).getExpr())
 }