Address review comments

Author: knewbury01

change_notes/2026-02-03-uninitialized-mem-improve.md

@@ -1,2 +1,2 @@
 - `A8-5-0`, `EXP53-CPP`, `EXP33-C`, `RULE-9-1` - `MemoryNotInitializedBeforeItIsRead.ql`, `DoNotReadUninitializedMemory.ql`, `DoNotReadUninitializedMemory.ql`, `ObjectWithAutoStorageDurationReadBeforeInit.ql`:
-  - The queries listed now find uses of the operator 'new' where there is no value initialization provided.
+  - The queries listed now find uses of the operator 'new' where there is no value initialization provided. The queries listed now also uses an out of the box library to consider initialization within another function as valid initialization (`InitializationFunctions.qll`). We do not yet track finely track the initialization/use of `p` vs `*p`.

cpp/common/src/codingstandards/cpp/lifetimes/CppObjects.qll

@@ -246,7 +246,7 @@ class AggregateLiteralObjectIdentity extends AggregateLiteral, ObjectIdentityBas
 }
 
 /**
- * An object identified by a call to `malloc`.
+ * An object identified by a call to `malloc` or allcoated with a `new` or `new[]` expression.
  *
  * Note: the malloc expression returns an address to this object, not the object itself. Therefore,
  * `getAnAccess()` returns cases where this malloc result is dereferenced, and not the malloc call

cpp/common/src/codingstandards/cpp/rules/readofuninitializedmemory/ReadOfUninitializedMemory.qll

@@ -122,6 +122,10 @@ class InitializationContext extends TInitializationContext {
   }
 }
 
+/**
+ * Catches `new int;` as an expression that doesn't initialize its value. Note that the pointer returned has been initialized (ie it is a valid pointer),
+ * but the pointee/value has not. In our analysis, we simply count `x` as uninitialized in `x = new int` for now, though a more thorough analysis might track the initialization of `x` and `*x` separately.
+ */
 class NewNotInit extends NewExpr {
   NewNotInit() {
     this.getAllocatedType() instanceof BuiltInType and
@@ -133,10 +137,6 @@ class NonInitAssignment extends Assignment {
   NonInitAssignment() { this.getRValue() instanceof NewNotInit }
 }
 
-class VariableAccessOnLHSOfNonInitAssignment extends VariableAccess {
-  VariableAccessOnLHSOfNonInitAssignment() { exists(NonInitAssignment a | this = a.getLValue()) }
-}
-
 /**
  * A local variable without an initializer which is amenable to initialization analysis.
  */
@@ -146,14 +146,7 @@ class UninitializedVariable extends LocalVariable {
     (
       not exists(getInitializer().getExpr())
       or
-      //or is a builtin type used with new operator but there is no value initialization as far as we can see
-      exists(Initializer i, NewExpr n |
-        i = getInitializer() and
-        n = i.getExpr() and
-        this.getUnspecifiedType().stripType() instanceof BuiltInType and
-        //ignore value init
-        not exists(n.getAChild())
-      )
+      getInitializer().getExpr() instanceof NewNotInit
     ) and
     // Not static or thread local, because they are not initialized with indeterminate values
     not isStatic() and
@@ -213,17 +206,32 @@ class UninitializedVariable extends LocalVariable {
     // Not a pointless read
     not result = any(ExprStmt es).getExpr() and
     // not involved in a new expr assignment since that does not define
-    not result instanceof VariableAccessOnLHSOfNonInitAssignment
+    not result = any(NonInitAssignment a).getLValue()
   }
 
   /**
-   * Gets an access of the variable `v` which is not used as an lvalue, and not used as an argument
+   * Gets an access of the this variable which is not used as an lvalue, and not used as an argument
    * to an initialization function.
    */
   VariableAccess getAUse() {
     result = this.getAnAccess() and
-    // Not used as an lvalue
-    not result = any(AssignExpr a).getLValue() and
+    (
+      //count rvalue x as a use if not new int
+      result.isRValue() and
+      not exists(PointerDereferenceExpr e | result = e.getAChild()) and
+      not this.getInitializer().getExpr() instanceof NewNotInit
+      or
+      //count lvalue x as a use if used in *x and not new int
+      result.isLValue() and
+      exists(PointerDereferenceExpr e | result = e.getAChild()) and
+      exists(this.getInitializer()) and
+      not this.getInitializer().getExpr() instanceof NewNotInit
+      or
+      //count rvalue *x as a use if has new int
+      result.isRValue() and
+      this.getInitializer().getExpr() instanceof NewNotInit and
+      exists(PointerDereferenceExpr e | result = e.getAChild())
+    ) and
     // Not passed to another initialization function
     not exists(Call c, int j | j = c.getTarget().(InitializationFunction).initializedParameter() |
       result = c.getArgument(j).(AddressOfExpr).getOperand()
@@ -234,6 +242,26 @@ class UninitializedVariable extends LocalVariable {
     not result.getParent+() instanceof SizeofOperator
   }
 
+  // /**
+  //  * Gets an access of the this variable which is not used as an lvalue, and not used as an argument
+  //  * to an initialization function.
+  //  */
+  // VariableAccess getAUse() {
+  //   result = this.getAnAccess() and
+  //   // Not used as an lvalue
+  //   not result = any(AssignExpr a).getLValue() and
+  //   //(result.isRValue() and not result.getType() instanceof PointerType and not this.getInitializer().getExpr() instanceof NewNotInit) and
+  //   // Not passed to another initialization function
+  //   not exists(Call c, int j | j = c.getTarget().(InitializationFunction).initializedParameter() |
+  //     result = c.getArgument(j).(AddressOfExpr).getOperand()
+  //     or
+  //     result.isRValue() and result = c.getArgument(j)
+  //   ) and
+  //   // Not a pointless read
+  //   not result = any(ExprStmt es).getExpr() and
+  //   // sizeof operators are not real uses
+  //   not result.getParent+() instanceof SizeofOperator
+  // }
   /** Get a read of the variable that may occur while the variable is uninitialized. */
   VariableAccess getAnUnitializedUse() {
     exists(SubBasicBlock useSbb |

cpp/common/test/rules/readofuninitializedmemory/ReadOfUninitializedMemory.expected

@@ -6,10 +6,11 @@
 | test.cpp:134:11:134:11 | i | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:133:7:133:7 | i | i |
 | test.cpp:137:13:137:14 | i1 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:136:8:136:9 | i1 | i1 |
 | test.cpp:141:7:141:8 | i3 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:139:8:139:9 | i3 | i3 |
-| test.cpp:141:13:141:14 | i1 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:136:8:136:9 | i1 | i1 |
-| test.cpp:201:7:201:8 | p0 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:200:8:200:9 | p0 | p0 |
-| test.cpp:204:4:204:5 | p1 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:203:8:203:9 | p1 | p1 |
-| test.cpp:206:7:206:8 | p1 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:203:8:203:9 | p1 | p1 |
-| test.cpp:211:7:211:8 | p2 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:209:8:209:9 | p2 | p2 |
-| test.cpp:214:10:214:11 | p2 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:209:8:209:9 | p2 | p2 |
-| test.cpp:221:7:221:8 | p4 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:219:8:219:9 | p4 | p4 |
+| test.cpp:204:8:204:9 | p0 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:202:8:202:9 | p0 | p0 |
+| test.cpp:207:4:207:5 | p1 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:206:8:206:9 | p1 | p1 |
+| test.cpp:211:8:211:9 | p1 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:206:8:206:9 | p1 | p1 |
+| test.cpp:217:8:217:9 | p2 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:214:8:214:9 | p2 | p2 |
+| test.cpp:220:10:220:11 | p2 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:214:8:214:9 | p2 | p2 |
+| test.cpp:229:7:229:8 | p4 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:227:8:227:9 | p4 | p4 |
+| test.cpp:233:14:233:15 | p5 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:232:8:232:9 | p5 | p5 |
+| test.cpp:237:8:237:9 | p6 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:235:8:235:9 | p6 | p6 |

cpp/common/test/rules/readofuninitializedmemory/test.cpp

@@ -196,28 +196,46 @@ void test_class() {
   }
 }
 
+void initialize(int *p) { *p = 0; }
+
 void extra_extra_test() {
   int *p0 = new int;
-  use(p0); // NON_COMPLIANT
+  use(p0);  // COMPLIANT -- the pointer is valid
+  use(*p0); // COMPLIANT[FALSE_POSITIVE] -- the pointer is valid
 
   int *p1 = new int;
-  *p1 = 0; // COMPLIANT[FALSE_POSITIVE] -- this is not found bc this is not an
-           // lvalue access
-  use(p1); // COMPLIANT[FALSE_POSITIVE] -- the pointee of p1 has been
-           // initialized
+  *p1 = 0;  // COMPLIANT[FALSE_POSITIVE] -- this is not found bc this is not an
+            // lvalue access
+  use(p1);  // COMPLIANT -- the pointee of p1 has been
+            // initialized
+  use(*p1); // COMPLIANT[FALSE_POSITIVE] -- the pointee of p1 has been
+            // initialized
 
   int *p2 = new int;
   p2 = new int;
-  use(p2); // NON_COMPLIANT
+  use(p2);  // COMPLIANT -- the pointer is valid
+  use(*p2); // NON_COMPLIANT -- the value may be read
 
   int *p3 = new int(1);
   *p3 = *p2; // NON_COMPLIANT -- the pointee of p2 has not been
              // initialized
-  use(p3);   // NON_COMPLIANT[FALSE_NEGATIVE] -- the pointee of p3 has be
+  use(p3);   // NON_COMPLIANT[FALSE_NEGATIVE] -- the pointee of p3 has been
+             // overridden
+  use(*p3);  // NON_COMPLIANT[FALSE_NEGATIVE] -- the pointee of p3 has been
              // overridden
 
   int *p4;
   p4 = new int;
-  use(p4); // NON_COMPLIANT -- the pointee of p4 has not been
-           // initialized
+  use(p4);  // COMPLIANT[FALSE_POSITIVE] -- the pointer is valid but new int isnt seen
+  use(*p4); // COMPLIANT -- the value is not read and the pointer is valid
+
+  int *p5;
+  initialize(p5); // NON_COMPLIANT
+
+  int *p6 = new int;
+  initialize(p6); // COMPLIANT
+  use(*p6); // COMPLIANT[FALSE_POSITIVE]
+
+  int p7;
+  initialize(&p7); // COMPLIANT
 }

cpp/misra/src/rules/RULE-6-8-3/AutomaticStorageAssignedToObjectGreaterLifetime.ql

@@ -1,6 +1,6 @@
 /**
  * @id cpp/misra/automatic-storage-assigned-to-object-greater-lifetime
- * @name RULE-6-8-3: Declare objects with appropriate storage durations
+ * @name RULE-6-8-3: Do not assign the address of an object with automatic storage to an object that may persist after it's lifetime
  * @description When storage durations are not compatible between assigned pointers it can lead to
  *              referring to objects outside of their lifetime, which is undefined behaviour.
  * @kind problem

rule_packages/cpp/Lifetime.json

@@ -18,7 +18,10 @@
             "correctness",
             "security",
             "scope/system"
-          ]
+          ],
+          "implementation_scope": {
+            "description": "The rule currently does not track member initialization or arrays at all (that have been declared with array types when they have not been assigned via pointers)."
+          }
         }
       ],
       "title": "The value of an object must not be read before it has been set"
@@ -32,7 +35,7 @@
         {
           "description": "When storage durations are not compatible between assigned pointers it can lead to referring to objects outside of their lifetime, which is undefined behaviour.",
           "kind": "problem",
-          "name": "Declare objects with appropriate storage durations",
+          "name": "Do not assign the address of an object with automatic storage to an object that may persist after it's lifetime",
           "precision": "very-high",
           "severity": "error",
           "short_name": "AutomaticStorageAssignedToObjectGreaterLifetime",