Address next round of feedback

Author: MichaelRFairhurst

c/misra/src/rules/RULE-18-10/PointersToVariablyModifiedArrayTypesUsed.ql

@@ -42,7 +42,9 @@ where
     if v.getType().(PointerType).getBaseType() instanceof CandidateVlaType
     then relationstr = "pointer to"
     else relationstr = "with inner"
-  )
+  ) and
+  // Remove results that appear to be unreliable, potentially from a macro.
+  not v.appearsDuplicated()
 select v,
   declstr + v.getName() + " is " + adjuststr + " variably-modified type, " + relationstr +
     " variable length array of non constant size $@ and element type '" +

c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql

@@ -15,16 +15,55 @@
 import cpp
 import codingstandards.c.misra
 
-from VlaDeclStmt v, Expr size, ArrayType arrayType, string typeStr
+/**
+ * Typedefs may be declared as VLAs, eg, `typedef int vla[x];`. This query finds types that refer to
+ * such typedef types, for instance `vla foo;` or adding a dimension via `vla bar[10];`.
+ *
+ * Consts and other specifiers may be added, but `vla *ptr;` is not a VLA any more, and is excluded.
+ */
+class VlaTypedefType extends Type {
+  VlaDeclStmt vlaDecl;
+  ArrayType arrayType;
+
+  VlaTypedefType() {
+    // Holds for direct references to the typedef type:
+    this = vlaDecl.getType() and
+    vlaDecl.getType() instanceof TypedefType and
+    arrayType = vlaDecl.getType().stripTopLevelSpecifiers()
+    or
+    // Holds for adding a constant dimension to a VLA typedef type:
+    arrayType = this.stripTopLevelSpecifiers() and
+    vlaDecl = arrayType.getBaseType().(VlaTypedefType).getVlaDeclStmt()
+    or
+    // Carefully ignore specifiers, `stripTopLevelSpecifiers()` resolves past the typedef
+    exists(SpecifiedType st, VlaTypedefType inner |
+      st = this and
+      st.getBaseType() = inner and
+      arrayType = inner.getArrayType() and
+      vlaDecl = inner.getVlaDeclStmt()
+    )
+  }
+
+  VlaDeclStmt getVlaDeclStmt() { result = vlaDecl }
+
+  ArrayType getArrayType() { result = arrayType }
+}
+
+from Variable v, Expr size, ArrayType arrayType, VlaDeclStmt vlaDecl, string typeStr
 where
   not isExcluded(v, Declarations7Package::variableLengthArrayTypesUsedQuery()) and
-  size = v.getVlaDimensionStmt(0).getDimensionExpr() and
+  size = vlaDecl.getVlaDimensionStmt(0).getDimensionExpr() and
   (
     // Holds is if v is a variable declaration:
-    arrayType = v.getVariable().getType().stripTopLevelSpecifiers()
+    v = vlaDecl.getVariable() and
+    arrayType = v.getType().stripTopLevelSpecifiers()
     or
     // Holds is if v is a typedef declaration:
-    arrayType = v.getType().stripTopLevelSpecifiers()
+    exists(VlaTypedefType typedef |
+      v.getType() = typedef and
+      arrayType = typedef.getArrayType() and
+      vlaDecl = typedef.getVlaDeclStmt()
+    )
   ) and
   typeStr = arrayType.getBaseType().toString()
 select v, "Variable length array of element type '" + typeStr + "' with non-constant size $@.",

c/misra/src/rules/RULE-18-9/ArrayToPointerConversionOfTemporaryObject.ql

@@ -66,5 +66,5 @@ where
   fa.getTemporary() = temporary and
   conversion.getExpr() = fa and
   isUsedOrStored(temporaryObjectFlowStep*(conversion.getExpr()))
-select conversion, "Array to pointer conversion of array $@ from temporary object $@",
+select conversion, "Array to pointer conversion of array $@ from temporary object $@.",
   fa.getTarget(), fa.getTarget().getName(), temporary, temporary.toString()

c/misra/test/rules/RULE-18-10/test.c

@@ -92,4 +92,14 @@ void f2(int (*p1)[3],    // COMPLIANT
         int (*p3)[2][*], // NON-COMPLIANT[FALSE_NEGATIVE]
         int (*p4)[*][2], // NON-COMPLIANT[FALSE_NEGATIVE]
         int (*p5)[*][*]  // NON-COMPLIANT[FALSE_NEGATIVE]
-);
+);
+
+#define CONFUSING_MACRO() \
+  int x; \
+  int (*vla)[x]; \
+  int (*not_vla)[];
+
+void f3() {
+  // We cannot report `vla` in this macro without a false positive for `not_vla`.
+  CONFUSING_MACRO() // COMPLIANT
+}

c/misra/test/rules/RULE-18-8/test.c

@@ -14,9 +14,13 @@ void f(int n) {
 
   extern int e1[]; // COMPLIANT
 
-  // A typedef is not a VLA. However, `VlaDeclStmt`s match the typedef.
-  typedef int vlaTypedef[n]; // COMPLIANT[FALSE_POSITIVE]
-  vlaTypedef t1;             // NON_COMPLIANT[FALSE_NEGATIVE]
+  // A typedef is not a VLA.
+  typedef int vlaTypedef[n]; // COMPLIANT
+  // The declarations using the typedef may or may not be VLAs.
+  vlaTypedef t1;    // NON_COMPLIANT
+  vlaTypedef t2[1]; // NON_COMPLIANT
+  vlaTypedef t3[x]; // NON_COMPLIANT
+  vlaTypedef *t4;   // COMPLIANT
 }
 
 void f1(int n,

cpp/common/src/codingstandards/cpp/VariablyModifiedTypes.qll

@@ -2,6 +2,8 @@ import cpp
 
 /**
  * A declaration involving a variably-modified type.
+ *
+ * Note, this holds for both VLA variable and VLA typedefs.
  */
 class VmtDeclarationEntry extends DeclarationEntry {
   Expr sizeExpr;
@@ -28,6 +30,14 @@ class VmtDeclarationEntry extends DeclarationEntry {
   Expr getSizeExpr() { result = sizeExpr }
 
   CandidateVlaType getVlaType() { result = vlaType }
+
+  /* VLAs may occur in macros, and result in duplication that messes up our analysis. */
+  predicate appearsDuplicated() {
+    exists(VmtDeclarationEntry other |
+      other != this and
+      other.getSizeExpr() = getSizeExpr()
+    )
+  }
 }
 
 /**