From f9b19c9d4caaf870b30cce8a3d6be79eda099c4e Mon Sep 17 00:00:00 2001
From: Frantisek Sumsal <frantisek@sumsal.cz>
Date: Sun, 5 Dec 2021 16:11:35 +0100
Subject: [PATCH] lgtm: detect more possible problematic scenarios

1) don't ignore stack-allocated variables, since they may hide
   heap-allocated stuff (compound types)
2) check if there's a return between the variable declaration and its
   initialization; if so, treat the variable as uninitialized
3) introduction of 2) increased the query runtime exponentially, so
   introduce some optimizations to bring it back to some reasonable
   values

(cherry picked from commit c8fec8bf9b086f9fc7638db0f1a613a00d7c63a3)

Related: #2017033
---
 .../UninitializedVariableWithCleanup.ql       | 48 ++++++++++---------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql b/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql
index 8c24b6d8f1..6b3b62f8bc 100644
--- a/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql
+++ b/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql
@@ -16,24 +16,6 @@
 import cpp
 import semmle.code.cpp.controlflow.StackVariableReachability
 
-/**
- * Auxiliary predicate: Types that don't require initialization
- * before they are used, since they're stack-allocated.
- */
-predicate allocatedType(Type t) {
-  /* Arrays: "int foo[1]; foo[0] = 42;" is ok. */
-  t instanceof ArrayType
-  or
-  /* Structs: "struct foo bar; bar.baz = 42" is ok. */
-  t instanceof Class
-  or
-  /* Typedefs to other allocated types are fine. */
-  allocatedType(t.(TypedefType).getUnderlyingType())
-  or
-  /* Type specifiers don't affect whether or not a type is allocated. */
-  allocatedType(t.getUnspecifiedType())
-}
-
 /** Auxiliary predicate: List cleanup functions we want to explicitly ignore
   * since they don't do anything illegal even when the variable is uninitialized
   */
@@ -47,13 +29,11 @@ predicate cleanupFunctionDenyList(string fun) {
  */
 DeclStmt declWithNoInit(LocalVariable v) {
   result.getADeclaration() = v and
-  not exists(v.getInitializer()) and
+  not v.hasInitializer() and
   /* The variable has __attribute__((__cleanup__(...))) set */
   v.getAnAttribute().hasName("cleanup") and
   /* Check if the cleanup function is not on a deny list */
-  not exists(Attribute a | a = v.getAnAttribute() and a.getName() = "cleanup" | cleanupFunctionDenyList(a.getAnArgument().getValueText())) and
-  /* The type of the variable is not stack-allocated. */
-  exists(Type t | t = v.getType() | not allocatedType(t))
+  not cleanupFunctionDenyList(v.getAnAttribute().getAnArgument().getValueText())
 }
 
 class UninitialisedLocalReachability extends StackVariableReachability {
@@ -78,7 +58,29 @@ class UninitialisedLocalReachability extends StackVariableReachability {
   override predicate isBarrier(ControlFlowNode node, StackVariable v) {
     // only report the _first_ possibly uninitialized use
     useOfVar(v, node) or
-    definitionBarrier(v, node)
+    (
+      /* If there's an return statement somewhere between the variable declaration
+       * and a possible definition, don't accept is as a valid initialization.
+       *
+       * E.g.:
+       * _cleanup_free_ char *x;
+       * ...
+       * if (...)
+       *    return;
+       * ...
+       * x = malloc(...);
+       *
+       * is not a valid initialization, since we might return from the function
+       * _before_ the actual iniitialization (emphasis on _might_, since we
+       * don't know if the return statement might ever evaluate to true).
+       */
+      definitionBarrier(v, node) and
+      not exists(ReturnStmt rs |
+                 /* The attribute check is "just" a complexity optimization */
+                 v.getFunction() = rs.getEnclosingFunction() and v.getAnAttribute().hasName("cleanup") |
+                 rs.getLocation().isBefore(node.getLocation())
+      )
+    )
   }
 }