Merge pull request #684 from github/lcartey/rule-12-2-improvements

`RULE-12-2`: Address common false positives and improve description

Author: lcartey

c/misra/src/codingstandards/c/misra/EssentialTypes.qll

@@ -355,13 +355,17 @@ class EssentialLiteral extends EssentialExpr, Literal {
     else (
       if this.(CharLiteral).getCharacter().length() = 1
       then result instanceof PlainCharType
-      else (
-        getStandardType().(IntegralType).isSigned() and
-        result = stlr(this)
-        or
-        not getStandardType().(IntegralType).isSigned() and
-        result = utlr(this)
-      )
+      else
+        exists(Type underlyingStandardType |
+          underlyingStandardType = getStandardType().getUnderlyingType()
+        |
+          if underlyingStandardType instanceof IntType
+          then
+            if underlyingStandardType.(IntType).isSigned()
+            then result = stlr(this)
+            else result = utlr(this)
+          else result = underlyingStandardType
+        )
     )
   }
 }

c/misra/src/rules/RULE-12-2/RightHandOperandOfAShiftRange.ql

@@ -20,14 +20,51 @@ class ShiftExpr extends BinaryBitwiseOperation {
   ShiftExpr() { this instanceof LShiftExpr or this instanceof RShiftExpr }
 }
 
-from ShiftExpr e, Expr right, int max_val
+MacroInvocation getAMacroInvocation(ShiftExpr se) { result.getAnExpandedElement() = se }
+
+Macro getPrimaryMacro(ShiftExpr se) {
+  exists(MacroInvocation mi |
+    mi = getAMacroInvocation(se) and
+    not exists(MacroInvocation otherMi |
+      otherMi = getAMacroInvocation(se) and otherMi.getParentInvocation() = mi
+    ) and
+    result = mi.getMacro()
+  )
+}
+
+from
+  ShiftExpr e, Expr right, int max_val, float lowerBound, float upperBound, Type essentialType,
+  string extraMessage, Locatable optionalPlaceholderLocation, string optionalPlaceholderMessage
 where
   not isExcluded(right, Contracts7Package::rightHandOperandOfAShiftRangeQuery()) and
   right = e.getRightOperand().getFullyConverted() and
-  max_val = (8 * getEssentialType(e.getLeftOperand()).getSize()) - 1 and
+  essentialType = getEssentialType(e.getLeftOperand()) and
+  max_val = (8 * essentialType.getSize()) - 1 and
+  upperBound = upperBound(right) and
+  lowerBound = lowerBound(right) and
+  (
+    lowerBound < 0 or
+    upperBound > max_val
+  ) and
+  // If this shift happens inside a macro, then report the macro as well
+  // for easier validation
   (
-    lowerBound(right) < 0 or
-    upperBound(right) > max_val
+    if exists(getPrimaryMacro(e))
+    then
+      extraMessage = " from expansion of macro $@" and
+      exists(Macro m |
+        m = getPrimaryMacro(e) and
+        optionalPlaceholderLocation = m and
+        optionalPlaceholderMessage = m.getName()
+      )
+    else (
+      extraMessage = "" and
+      optionalPlaceholderLocation = e and
+      optionalPlaceholderMessage = ""
+    )
   )
 select right,
-  "The right hand operand of the shift operator shall lie in the range 0 to " + max_val + "."
+  "The possible range of the right operand of the shift operator (" + lowerBound + ".." + upperBound
+    + ") is outside the the valid shift range (0.." + max_val +
+    ") for the essential type of the left operand (" + essentialType + ")" + extraMessage + ".",
+  optionalPlaceholderLocation, optionalPlaceholderMessage

c/misra/test/c/misra/EssentialTypes.expected

@@ -38,3 +38,6 @@
 | test.c:26:3:26:3 | f | float | float | essentially Floating type |
 | test.c:27:3:27:5 | f32 | float32_t | float32_t | essentially Floating type |
 | test.c:28:3:28:6 | cf32 | float | float | essentially Floating type |
+| test.c:32:3:32:3 | 1 | signed char | signed char | essentially Signed type |
+| test.c:33:3:33:4 | 1 | unsigned char | unsigned char | essentially Unsigned type |
+| test.c:34:3:34:5 | 1 | unsigned long | unsigned long | essentially Unsigned type |

c/misra/test/c/misra/test.c

@@ -26,4 +26,10 @@ void testCategoriesForComplexTypes() {
   f;    // Should be essentially Floating type
   f32;  // Should be essentially Floating type
   cf32; // Should be essentially Floating type
+}
+
+void testConstants() {
+  1;   // Essentially signed char
+  1U;  // Essentially unsigned char
+  1UL; // Essentially unsigned long
 }

c/misra/test/rules/RULE-12-2/RightHandOperandOfAShiftRange.expected

@@ -1,10 +1,12 @@
-| test.c:8:10:8:10 | 8 | The right hand operand of the shift operator shall lie in the range 0 to 7. |
-| test.c:9:10:9:11 | - ... | The right hand operand of the shift operator shall lie in the range 0 to 7. |
-| test.c:10:10:10:14 | ... + ... | The right hand operand of the shift operator shall lie in the range 0 to 7. |
-| test.c:11:10:11:14 | ... + ... | The right hand operand of the shift operator shall lie in the range 0 to 7. |
-| test.c:13:21:13:22 | 16 | The right hand operand of the shift operator shall lie in the range 0 to 15. |
-| test.c:16:9:16:9 | 8 | The right hand operand of the shift operator shall lie in the range 0 to 7. |
-| test.c:21:9:21:10 | 64 | The right hand operand of the shift operator shall lie in the range 0 to 63. |
-| test.c:25:10:25:10 | 8 | The right hand operand of the shift operator shall lie in the range 0 to 7. |
-| test.c:26:10:26:11 | 64 | The right hand operand of the shift operator shall lie in the range 0 to 7. |
-| test.c:30:16:30:17 | 64 | The right hand operand of the shift operator shall lie in the range 0 to 63. |
+| test.c:8:10:8:10 | 8 | The possible range of the right operand of the shift operator (8..8) is outside the the valid shift range (0..7) for the essential type of the left operand (uint8_t). | test.c:8:3:8:10 | ... >> ... |  |
+| test.c:9:10:9:11 | - ... | The possible range of the right operand of the shift operator (-1..-1) is outside the the valid shift range (0..7) for the essential type of the left operand (uint8_t). | test.c:9:3:9:11 | ... >> ... |  |
+| test.c:10:10:10:14 | ... + ... | The possible range of the right operand of the shift operator (8..8) is outside the the valid shift range (0..7) for the essential type of the left operand (uint8_t). | test.c:10:3:10:14 | ... >> ... |  |
+| test.c:11:10:11:14 | ... + ... | The possible range of the right operand of the shift operator (8..8) is outside the the valid shift range (0..7) for the essential type of the left operand (uint8_t). | test.c:11:3:11:14 | ... << ... |  |
+| test.c:13:21:13:22 | 16 | The possible range of the right operand of the shift operator (16..16) is outside the the valid shift range (0..15) for the essential type of the left operand (uint16_t). | test.c:13:3:13:22 | ... << ... |  |
+| test.c:16:9:16:9 | 8 | The possible range of the right operand of the shift operator (8..8) is outside the the valid shift range (0..7) for the essential type of the left operand (unsigned char). | test.c:16:3:16:9 | ... << ... |  |
+| test.c:21:9:21:10 | 64 | The possible range of the right operand of the shift operator (64..64) is outside the the valid shift range (0..63) for the essential type of the left operand (unsigned long). | test.c:21:3:21:10 | ... << ... |  |
+| test.c:26:10:26:11 | 64 | The possible range of the right operand of the shift operator (64..64) is outside the the valid shift range (0..63) for the essential type of the left operand (unsigned long). | test.c:26:3:26:11 | ... << ... |  |
+| test.c:30:16:30:17 | 64 | The possible range of the right operand of the shift operator (64..64) is outside the the valid shift range (0..63) for the essential type of the left operand (unsigned long). | test.c:30:3:30:17 | ... << ... |  |
+| test.c:34:8:34:8 | y | The possible range of the right operand of the shift operator (0..4294967295) is outside the the valid shift range (0..31) for the essential type of the left operand (unsigned int). | test.c:34:3:34:8 | ... >> ... |  |
+| test.c:40:8:40:8 | y | The possible range of the right operand of the shift operator (-2147483648..2147483647) is outside the the valid shift range (0..31) for the essential type of the left operand (signed int). | test.c:40:3:40:8 | ... >> ... |  |
+| test.c:42:8:42:8 | y | The possible range of the right operand of the shift operator (-31..31) is outside the the valid shift range (0..31) for the essential type of the left operand (signed int). | test.c:42:3:42:8 | ... >> ... |  |

c/misra/test/rules/RULE-12-2/test.c

@@ -29,3 +29,15 @@ void f1() {
   ULONG_MAX << 8;  // COMPLIANT
   ULONG_MAX << 64; // NON_COMPLIANT
 }
+
+void unsignedRemAssign(unsigned int y, unsigned int x) {
+  x >> y; // NON_COMPLIANT
+  y %= 32;
+  x >> y; // COMPLIANT
+}
+
+void signedRemAssign(signed int y, signed int x) {
+  x >> y; // NON_COMPLIANT
+  y %= 32;
+  x >> y; // NON_COMPLIANT - may be negative
+}

change_notes/2024-09-11-rule-12-2-improvements.md

@@ -0,0 +1,6 @@
+- `RULE-12-2` - `RightHandOperandOfAShiftRange.ql`:
+  - Reduce false positives related to ranges determined by `%=`.
+  - Reduce false positives for integer constants with explicit size suffix were incorrectly identified as smaller types.
+  - Improve explanation of results, providing additional information on types and size ranges.
+  - Combine results stemming from the expansion of a macro, where the result is not dependent on the context.
+  

cpp/common/src/codingstandards/cpp/SimpleRangeAnalysisCustomizations.qll

@@ -151,6 +151,40 @@ private class CastEnumToIntegerSimpleRange extends SimpleRangeAnalysisExpr, Cast
   override predicate dependsOnChild(Expr child) { child = getExpr() }
 }
 
+/**
+ * A range analysis extension that supports `%=`.
+ */
+private class RemAssignSimpleRange extends SimpleRangeAnalysisExpr, AssignRemExpr {
+  override float getLowerBounds() {
+    exists(float maxDivisorNegated, float dividendLowerBounds |
+      // Find the max divisor, negated e.g. `%= 32` would be  `-31`
+      maxDivisorNegated = (getFullyConvertedUpperBounds(getRValue()).abs() - 1) * -1 and
+      // Find the lower bounds of the dividend
+      dividendLowerBounds = getFullyConvertedLowerBounds(getLValue()) and
+      // The lower bound is calculated in two steps:
+      // 1. Determine the maximum of the dividend lower bound and maxDivisorNegated.
+      //    When the dividend is negative this will result in a negative result
+      // 2. Find the minimum with 0. If the divided is always >0 this will produce 0
+      //    otherwise it will produce the lowest negative number that can be held
+      //    after the modulo.
+      result = 0.minimum(dividendLowerBounds.maximum(maxDivisorNegated))
+    )
+  }
+
+  override float getUpperBounds() {
+    exists(float maxDivisor, float maxDividend |
+      // The maximum divisor value is the absolute value of the divisor minus 1
+      maxDivisor = getFullyConvertedUpperBounds(getRValue()).abs() - 1 and
+      // value if > 0 otherwise 0
+      maxDividend = getFullyConvertedUpperBounds(getLValue()).maximum(0) and
+      // In the case the numerator is definitely less than zero, the result could be negative
+      result = maxDividend.minimum(maxDivisor)
+    )
+  }
+
+  override predicate dependsOnChild(Expr expr) { expr = getAChild() }
+}
+
 /**
  * <stdio.h> functions that read a character from the STDIN,
  * or return EOF if it fails to do so.