Merge pull request #17481 from github/calumgrant/bmn/uninitialized-local

C++: Remove FPs from cpp/uninitialized-local when encountered extraction errors

Author: calumgrant

cpp/ql/lib/semmle/code/cpp/Function.qll

@@ -500,6 +500,17 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function {
    * Gets the nearest enclosing AccessHolder.
    */
   override AccessHolder getEnclosingAccessHolder() { result = this.getDeclaringType() }
+
+  /**
+   * Holds if this function has extraction errors that create an `ErrorExpr`.
+   */
+  predicate hasErrors() {
+    exists(ErrorExpr e |
+      e.getEnclosingFunction() = this and
+      // Exclude the first allocator call argument because it is always extracted as `ErrorExpr`.
+      not exists(NewOrNewArrayExpr new | e = new.getAllocatorCall().getArgument(0))
+    )
+  }
 }
 
 pragma[noinline]

cpp/ql/src/Best Practices/Unused Entities/UnusedLocals.ql

@@ -57,5 +57,5 @@ where
   not declarationHasSideEffects(v) and
   not exists(AsmStmt s | f = s.getEnclosingFunction()) and
   not v.getAnAttribute().getName() = "unused" and
-  not any(ErrorExpr e).getEnclosingFunction() = f // unextracted expr may use `v`
+  not f.hasErrors() // Unextracted expressions may use `v`
 select v, "Variable " + v.getName() + " is not used."

cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql

@@ -29,7 +29,7 @@ class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration {
   override predicate isSource(Instruction source) {
     exists(Function func |
       // Rule out FPs caused by extraction errors.
-      not any(ErrorExpr e).getEnclosingFunction() = func and
+      not func.hasErrors() and
       not intentionallyReturnsStackPointer(func) and
       func = source.getEnclosingFunction()
     |

cpp/ql/src/Likely Bugs/Memory Management/UninitializedLocal.ql

@@ -65,6 +65,7 @@ predicate isSinkImpl(Instruction sink, VariableAccess va) {
   exists(LoadInstruction load |
     va = load.getUnconvertedResultExpression() and
     not va = commonException() and
+    not va.getTarget().(LocalVariable).getFunction().hasErrors() and
     sink = load.getSourceValue()
   )
 }

cpp/ql/src/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql

@@ -24,7 +24,7 @@ predicate instructionHasVariable(VariableAddressInstruction vai, StackVariable v
   // Pointer-to-member types aren't properly handled in the dbscheme.
   not vai.getResultType() instanceof PointerToMemberType and
   // Rule out FPs caused by extraction errors.
-  not any(ErrorExpr e).getEnclosingFunction() = f
+  not f.hasErrors()
 }
 
 /**

cpp/ql/src/change-notes/2024-10-02-uninitialized-local.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Fixed false positives in the `cpp/uninitialized-local` ("Potentially uninitialized local variable") query if there are extraction errors in the function.

cpp/ql/src/jsf/4.13 Functions/AV Rule 114.ql

@@ -49,7 +49,7 @@ predicate functionsMissingReturnStmt(Function f, ControlFlowNode blame) {
 predicate functionImperfectlyExtracted(Function f) {
   exists(CompilerError e | f.getBlock().getLocation().subsumes(e.getLocation()))
   or
-  exists(ErrorExpr ee | ee.getEnclosingFunction() = f)
+  f.hasErrors()
   or
   count(f.getType()) > 1
   or

cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/UninitializedLocal.expected

@@ -1,5 +1,6 @@
 edges
 nodes
+| errors.cpp:13:7:13:7 | definition of x | semmle.label | definition of x |
 | test.cpp:11:6:11:8 | definition of foo | semmle.label | definition of foo |
 | test.cpp:111:6:111:8 | definition of foo | semmle.label | definition of foo |
 | test.cpp:226:7:226:7 | definition of x | semmle.label | definition of x |
@@ -14,6 +15,7 @@ nodes
 | test.cpp:472:6:472:6 | definition of x | semmle.label | definition of x |
 | test.cpp:479:6:479:6 | definition of x | semmle.label | definition of x |
 #select
+| errors.cpp:14:18:14:18 | x | errors.cpp:13:7:13:7 | definition of x | errors.cpp:13:7:13:7 | definition of x | The variable $@ may not be initialized at this access. | errors.cpp:13:7:13:7 | x | x |
 | test.cpp:12:6:12:8 | foo | test.cpp:11:6:11:8 | definition of foo | test.cpp:11:6:11:8 | definition of foo | The variable $@ may not be initialized at this access. | test.cpp:11:6:11:8 | foo | foo |
 | test.cpp:113:6:113:8 | foo | test.cpp:111:6:111:8 | definition of foo | test.cpp:111:6:111:8 | definition of foo | The variable $@ may not be initialized at this access. | test.cpp:111:6:111:8 | foo | foo |
 | test.cpp:227:3:227:3 | x | test.cpp:226:7:226:7 | definition of x | test.cpp:226:7:226:7 | definition of x | The variable $@ may not be initialized at this access. | test.cpp:226:7:226:7 | x | x |

cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/errors.cpp

@@ -0,0 +1,15 @@
+// semmle-extractor-options: --expect_errors
+
+int f1() {
+  int x;
+  initialize(&x); // error expression - initialize() is not defined
+  return x;       // GOOD - assume x is initialized
+}
+
+void * operator new(unsigned long, bool);
+void operator delete(void*, bool);
+
+int f2() {
+  int x;
+  new(true) int (x);  // BAD, ignore implicit error expression
+}