Resolve FP reported in 396

c/cert/src/rules/INT34-C/ExprShiftedbyNegativeOrGreaterPrecisionOperand.ql

@@ -15,91 +15,8 @@ import codingstandards.c.cert
 import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import semmle.code.cpp.controlflow.Guards
+import codingstandards.cpp.UndefinedBehavior
 
-/*
- * Precision predicate based on a sample implementation from
- * https://wiki.sei.cmu.edu/confluence/display/c/INT35-C.+Use+correct+integer+precisions
- */
-
-/**
- * A function whose name is suggestive that it counts the number of bits set.
- */
-class PopCount extends Function {
-  PopCount() { this.getName().toLowerCase().matches("%popc%nt%") }
-}
-
-/**
- * A macro which is suggestive that it is used to determine the precision of an integer.
- */
-class PrecisionMacro extends Macro {
-  PrecisionMacro() { this.getName().toLowerCase().matches("precision") }
-}
-
-class LiteralZero extends Literal {
-  LiteralZero() { this.getValue() = "0" }
-}
-
-class BitShiftExpr extends BinaryBitwiseOperation {
-  BitShiftExpr() {
-    this instanceof LShiftExpr or
-    this instanceof RShiftExpr
-  }
-}
-
-int getPrecision(IntegralType type) {
-  type.isExplicitlyUnsigned() and result = type.getSize() * 8
-  or
-  type.isExplicitlySigned() and result = type.getSize() * 8 - 1
-}
-
-predicate isForbiddenShiftExpr(BitShiftExpr shift, string message) {
-  (
-    (
-      getPrecision(shift.getLeftOperand().getExplicitlyConverted().getUnderlyingType()) <=
-        upperBound(shift.getRightOperand()) and
-      message =
-        "The operand " + shift.getLeftOperand() + " is shifted by an expression " +
-          shift.getRightOperand() + " whose upper bound (" + upperBound(shift.getRightOperand()) +
-          ") is greater than or equal to the precision."
-      or
-      lowerBound(shift.getRightOperand()) < 0 and
-      message =
-        "The operand " + shift.getLeftOperand() + " is shifted by an expression " +
-          shift.getRightOperand() + " which may be negative."
-    ) and
-    /*
-     * Shift statement is not at a basic block where
-     * `shift_rhs < PRECISION(...)` is ensured
-     */
-
-    not exists(GuardCondition gc, BasicBlock block, Expr precisionCall, Expr lTLhs |
-      block = shift.getBasicBlock() and
-      (
-        precisionCall.(FunctionCall).getTarget() instanceof PopCount
-        or
-        precisionCall = any(PrecisionMacro pm).getAnInvocation().getExpr()
-      )
-    |
-      globalValueNumber(lTLhs) = globalValueNumber(shift.getRightOperand()) and
-      gc.ensuresLt(lTLhs, precisionCall, 0, block, true)
-    ) and
-    /*
-     * Shift statement is not at a basic block where
-     * `shift_rhs < 0` is ensured
-     */
-
-    not exists(GuardCondition gc, BasicBlock block, Expr literalZero, Expr lTLhs |
-      block = shift.getBasicBlock() and
-      literalZero instanceof LiteralZero
-    |
-      globalValueNumber(lTLhs) = globalValueNumber(shift.getRightOperand()) and
-      gc.ensuresLt(lTLhs, literalZero, 0, block, true)
-    )
-  )
-}
-
-from BinaryBitwiseOperation badShift, string message
-where
-  not isExcluded(badShift, Types1Package::exprShiftedbyNegativeOrGreaterPrecisionOperandQuery()) and
-  isForbiddenShiftExpr(badShift, message)
-select badShift, message
+from ShiftByNegativeOrGreaterPrecisionOperand badShift
+where not isExcluded(badShift, Types1Package::exprShiftedbyNegativeOrGreaterPrecisionOperandQuery())
+select badShift, badShift.getReason()

c/common/src/codingstandards/c/UndefinedBehavior.qll

@@ -25,4 +25,9 @@ class CUndefinedMainDefinition extends CUndefinedBehavior, Function {
     (this.getName() = "main" or this.getName().indexOf("____codeql_coding_standards") = 0) and
     not this instanceof C99MainFunction
   }
+
+  override string getReason() {
+    result =
+      "The behavior of the program is undefined because the main function is not defined according to the C standard."
+  }
 }

change_notes/2024-02-14-fix-fp-m0-1-4.md

@@ -0,0 +1,4 @@
+- `M0-1-4` - `SingleUseMemberPODVariable.ql`:
+  - Address FP reported in #388. Include aggregrate initialization as a use of a member.
+  - Include indirect initialization of members. For example, casting a pointer to a buffer to a struct pointer.
+  - Reformat the alert message to adhere to the style-guide.

change_notes/2024-02-21-fix-reported-fp-a4-7-1.md

@@ -0,0 +1,4 @@
+- `A4-7-1` -  `IntegerExpressionLeadToDataLoss.ql`:
+    - Address reported FP in #396. Exclude shift operations guarded to prevent undefined behavior that could lead to dataloss. 
+- `INT34-C` - `ExprShiftedbyNegativeOrGreaterPrecisionOperand.ql`:
+  - Format the alert message according to the style-guide.

cpp/autosar/src/rules/M0-1-4/SingleUseMemberPODVariable.ql

@@ -24,5 +24,5 @@ where
   not isExcluded(v, DeadCodePackage::singleUseMemberPODVariableQuery()) and
   isSingleUseNonVolatilePODVariable(v)
 select v,
-  "Member POD variable " + v.getName() + " in " + v.getDeclaringType().getName() + " is only $@.",
-  getSingleUse(v), "used once"
+  "Member POD variable '" + v.getName() + "' in '" + v.getDeclaringType().getName() +
+    "' is only $@.", getSingleUse(v), "used once"

cpp/autosar/src/rules/M0-1-4/SingleUsePODVariable.qll

@@ -10,23 +10,68 @@ private string getConstExprValue(Variable v) {
   v.isConstexpr()
 }
 
+/**
+ * Gets the number of uses of variable `v` in an opaque assignment, where an opaqua assignment for example a cast from one type to the other and `v` is assumed to be a member of the resulting type.
+ * e.g.,
+ * struct foo {
+ *  int bar;
+ * }
+ *
+ * struct foo * v = (struct foo*)buffer;
+ */
+Expr getIndirectSubObjectAssignedValue(MemberVariable subobject) {
+  // struct foo * ptr = (struct foo*)buffer;
+  exists(Struct someStruct, Variable instanceOfSomeStruct | someStruct.getAMember() = subobject |
+    instanceOfSomeStruct.getType().(PointerType).getBaseType() = someStruct and
+    exists(Cast assignedValue |
+      // Exclude cases like struct foo * v = nullptr;
+      not assignedValue.isImplicit() and
+      // `v` is a subobject of another type that reinterprets another object. We count that as a use of `v`.
+      assignedValue.getExpr() = instanceOfSomeStruct.getAnAssignedValue() and
+      result = assignedValue
+    )
+    or
+    // struct foo; read(..., (char *)&foo);
+    instanceOfSomeStruct.getType() = someStruct and
+    exists(Call externalInitializerCall, Cast castToCharPointer, int n |
+      externalInitializerCall.getArgument(n).(AddressOfExpr).getOperand() =
+        instanceOfSomeStruct.getAnAccess() and
+      externalInitializerCall.getArgument(n) = castToCharPointer.getExpr() and
+      castToCharPointer.getType().(PointerType).getBaseType().getUnspecifiedType() instanceof
+        CharType and
+      result = externalInitializerCall
+    )
+    or
+    // the object this subject is part of is initialized and we assumes this initializes the subobject.
+    instanceOfSomeStruct.getType() = someStruct and
+    result = instanceOfSomeStruct.getInitializer().getExpr()
+  )
+}
+
 /** Gets a "use" count according to rule M0-1-4. */
 int getUseCount(Variable v) {
-  exists(int initializers |
-    // We enforce that it's a POD type variable, so if it has an initializer it is explicit
-    (if v.hasInitializer() then initializers = 1 else initializers = 0) and
-    result =
-      initializers +
-        count(VariableAccess access | access = v.getAnAccess() and not access.isCompilerGenerated())
-        + count(UserProvidedConstructorFieldInit cfi | cfi.getTarget() = v) +
-        // For constexpr variables used as template arguments, we don't see accesses (just the
-        // appropriate literals). We therefore take a conservative approach and count the number of
-        // template instantiations that use the given constant, and consider each one to be a use
-        // of the variable
-        count(ClassTemplateInstantiation cti |
-          cti.getTemplateArgument(_).(Expr).getValue() = getConstExprValue(v)
-        )
-  )
+  // We enforce that it's a POD type variable, so if it has an initializer it is explicit
+  result =
+    count(getAUserInitializedValue(v)) +
+      count(VariableAccess access | access = v.getAnAccess() and not access.isCompilerGenerated()) +
+      // For constexpr variables used as template arguments, we don't see accesses (just the
+      // appropriate literals). We therefore take a conservative approach and count the number of
+      // template instantiations that use the given constant, and consider each one to be a use
+      // of the variable
+      count(ClassTemplateInstantiation cti |
+        cti.getTemplateArgument(_).(Expr).getValue() = getConstExprValue(v)
+      ) + count(getIndirectSubObjectAssignedValue(v))
+}
+
+Expr getAUserInitializedValue(Variable v) {
+  (
+    result = v.getInitializer().getExpr()
+    or
+    exists(UserProvidedConstructorFieldInit cfi | cfi.getTarget() = v and result = cfi.getExpr())
+    or
+    exists(ClassAggregateLiteral l | not l.isCompilerGenerated() | result = l.getAFieldExpr(v))
+  ) and
+  not result.isCompilerGenerated()
 }
 
 /** Gets a single use of `v`, if `isSingleUseNonVolatilePODVariable` holds. */

cpp/autosar/test/rules/A4-7-1/IntegerExpressionLeadToDataLoss.expected

@@ -10,3 +10,6 @@
 | test.cpp:22:12:22:16 | ... + ... | Binary expression ...+... may overflow. |
 | test.cpp:50:7:50:14 | ... + ... | Binary expression ...+... may overflow. |
 | test.cpp:62:8:62:10 | ... ++ | Binary expression ...++... may overflow. |
+| test.cpp:91:10:91:17 | ... << ... | Binary expression ...<<... may overflow. |
+| test.cpp:95:10:95:17 | ... << ... | Binary expression ...<<... may overflow. |
+| test.cpp:98:8:98:15 | ... << ... | Binary expression ...<<... may overflow. |

cpp/autosar/test/rules/A4-7-1/test.cpp

@@ -72,4 +72,29 @@ void test_pointer() {
   int *p = nullptr;
   p++; // COMPLIANT - not covered by this rule
   p--; // COMPLIANT - not covered by this rule
+}
+
+extern unsigned int popcount(unsigned int);
+#define PRECISION(x) popcount(x)
+void test_guarded_shifts(unsigned int p1, int p2) {
+  unsigned int l1;
+
+  if (p2 < popcount(p1) && p2 > 0) {
+    l1 = p1 << p2; // COMPLIANT
+  }
+
+  if (p2 < PRECISION(p1) && p2 > 0) {
+    l1 = p1 << p2; // COMPLIANT
+  }
+
+  if (p2 < popcount(p1)) {
+    l1 = p1 << p2; // NON_COMPLIANT - p2 could be negative
+  }
+
+  if (p2 > 0) {
+    l1 = p1 << p2; // NON_COMPLIANT - p2 could have a higher precision
+  }
+
+  l1 = p1 << p2; // NON_COMPLIANT - p2 may have a higher precision or could be
+                 // negative
 }

cpp/autosar/test/rules/M0-1-4/SingleUseMemberPODVariable.expected

@@ -1,9 +1,9 @@
-| test_global_or_namespace.cpp:16:7:16:7 | x | Member POD variable x in GA is only $@. | test_global_or_namespace.cpp:38:6:38:6 | x | used once |
-| test_global_or_namespace.cpp:54:7:54:7 | x | Member POD variable x in N1A is only $@. | test_global_or_namespace.cpp:76:6:76:6 | x | used once |
-| test_member.cpp:5:7:5:8 | m2 | Member POD variable m2 in A is only $@. | test_member.cpp:9:21:9:25 | constructor init of field m2 | used once |
-| test_member.cpp:6:7:6:8 | m3 | Member POD variable m3 in A is only $@. | test_member.cpp:10:23:10:24 | m3 | used once |
-| test_member.cpp:7:7:7:8 | m4 | Member POD variable m4 in A is only $@. | test_member.cpp:14:23:14:24 | m4 | used once |
-| test_member.cpp:18:9:18:11 | sm1 | Member POD variable sm1 in s1 is only $@. | test_member.cpp:23:6:23:8 | sm1 | used once |
-| test_member.cpp:36:7:36:8 | m1 | Member POD variable m1 in C is only $@. | test_member.cpp:39:21:39:22 | m1 | used once |
-| test_member.cpp:37:7:37:8 | m2 | Member POD variable m2 in C is only $@. | test_member.cpp:46:5:46:6 | m2 | used once |
-| test_member.cpp:55:5:55:6 | m3 | Member POD variable m3 in E<B> is only $@. | test_member.cpp:56:27:56:32 | constructor init of field m3 | used once |
+| test_global_or_namespace.cpp:16:7:16:7 | x | Member POD variable 'x' in 'GA' is only $@. | test_global_or_namespace.cpp:38:6:38:6 | x | used once |
+| test_global_or_namespace.cpp:54:7:54:7 | x | Member POD variable 'x' in 'N1A' is only $@. | test_global_or_namespace.cpp:76:6:76:6 | x | used once |
+| test_member.cpp:5:7:5:8 | m2 | Member POD variable 'm2' in 'A' is only $@. | test_member.cpp:9:21:9:25 | constructor init of field m2 | used once |
+| test_member.cpp:6:7:6:8 | m3 | Member POD variable 'm3' in 'A' is only $@. | test_member.cpp:10:23:10:24 | m3 | used once |
+| test_member.cpp:7:7:7:8 | m4 | Member POD variable 'm4' in 'A' is only $@. | test_member.cpp:14:23:14:24 | m4 | used once |
+| test_member.cpp:18:9:18:11 | sm1 | Member POD variable 'sm1' in 's1' is only $@. | test_member.cpp:23:6:23:8 | sm1 | used once |
+| test_member.cpp:36:7:36:8 | m1 | Member POD variable 'm1' in 'C' is only $@. | test_member.cpp:39:21:39:22 | m1 | used once |
+| test_member.cpp:37:7:37:8 | m2 | Member POD variable 'm2' in 'C' is only $@. | test_member.cpp:46:5:46:6 | m2 | used once |
+| test_member.cpp:55:5:55:6 | m3 | Member POD variable 'm3' in 'E<B>' is only $@. | test_member.cpp:56:27:56:32 | constructor init of field m3 | used once |

cpp/autosar/test/rules/M0-1-4/test_member.cpp

@@ -72,4 +72,62 @@ void test_e() { // Ensure that the template E is fully instantiated
   e2.getT();
 }
 
+void test_fp_reported_in_388() {
+  struct s1 {
+    int m1; // COMPLIANT
+  };
+
+  s1 l1 = {1}; // m1 is used here
+  l1.m1;
+}
+
+void test_array_initialized_members() {
+  struct s1 {
+    int m1; // COMPLIANT
+  };
+
+  struct s1 l1[] = {
+      {.m1 = 1},
+      {.m1 = 2},
+  };
+
+  l1[0].m1;
+}
+
+void test_indirect_assigned_members(void *opaque) {
+  struct s1 {
+    int m1; // COMPLIANT
+  };
+
+  struct s1 *p = (struct s1 *)opaque;
+  p->m1;
+
+  struct s2 {
+    int m1; // COMPLIANT
+  };
+
+  char buffer[sizeof(struct s2) + 8] = {0};
+  struct s2 *l2 = (struct s2 *)&buffer[8];
+  l2->m1;
+}
+
+void test_external_assigned_members(void (*fp)(unsigned char *)) {
+
+  struct s1 {
+    int m1; // COMPLIANT
+  };
+
+  struct s1 l1;
+  fp((unsigned char *)&l1);
+  l1.m1;
+
+  struct s2 {
+    int m1; // COMPLIANT
+  };
+
+  struct s2 (*copy_init)();
+  struct s2 l2 = copy_init();
+  l2.m1;
+}
+
 } // namespace test

cpp/common/src/codingstandards/cpp/Expr.qll

@@ -180,3 +180,11 @@ module MisraExpr {
     CValue() { isCValue(this) }
   }
 }
+
+/** A class representing left and right bitwise shift operations. */
+class BitShiftExpr extends BinaryBitwiseOperation {
+  BitShiftExpr() {
+    this instanceof LShiftExpr or
+    this instanceof RShiftExpr
+  }
+}

cpp/common/src/codingstandards/cpp/Function.qll

@@ -0,0 +1,10 @@
+/** A module to reason about functions, such as well-known functions. */
+
+import cpp
+
+/**
+ * A function whose name is suggestive that it counts the number of bits set.
+ */
+class PopCount extends Function {
+  PopCount() { this.getName().toLowerCase().matches("%popc%nt%") }
+}

cpp/common/src/codingstandards/cpp/Literals.qll

@@ -28,3 +28,7 @@ class Utf16StringLiteral extends StringLiteral {
 class Utf32StringLiteral extends StringLiteral {
   Utf32StringLiteral() { this.getValueText().regexpMatch("(?s)\\s*U\".*") }
 }
+
+class LiteralZero extends Literal {
+  LiteralZero() { this.getValue() = "0" }
+}

cpp/common/src/codingstandards/cpp/Macro.qll

@@ -88,3 +88,10 @@ class UserProvidedMacro extends Macro {
 class LibraryMacro extends Macro {
   LibraryMacro() { not this instanceof UserProvidedMacro }
 }
+
+/**
+ * A macro which is suggestive that it is used to determine the precision of an integer.
+ */
+class PrecisionMacro extends Macro {
+  PrecisionMacro() { this.getName().toLowerCase().matches("precision") }
+}

cpp/common/src/codingstandards/cpp/Overflow.qll

@@ -8,6 +8,8 @@ import SimpleRangeAnalysisCustomizations
 import semmle.code.cpp.controlflow.Guards
 import codingstandards.cpp.dataflow.TaintTracking
 import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+import codingstandards.cpp.Expr
+import codingstandards.cpp.UndefinedBehavior
 
 /**
  * An integer operation that may overflow, underflow or wrap.
@@ -40,7 +42,9 @@ class InterestingOverflowingOperation extends Operation {
     // Not within a macro
     not this.isAffectedByMacro() and
     // Ignore pointer arithmetic
-    not this instanceof PointerArithmeticOperation
+    not this instanceof PointerArithmeticOperation and
+    // In case of the shift operation, it must cause undefined behavior
+    (this instanceof BitShiftExpr implies this instanceof ShiftByNegativeOrGreaterPrecisionOperand)
   }
 
   /**