daandemeyer / rpms / systemd

Forked from rpms/systemd 2 years ago
Clone
3d3dc8
From f9b19c9d4caaf870b30cce8a3d6be79eda099c4e Mon Sep 17 00:00:00 2001
3d3dc8
From: Frantisek Sumsal <frantisek@sumsal.cz>
3d3dc8
Date: Sun, 5 Dec 2021 16:11:35 +0100
3d3dc8
Subject: [PATCH] lgtm: detect more possible problematic scenarios
3d3dc8
3d3dc8
1) don't ignore stack-allocated variables, since they may hide
3d3dc8
   heap-allocated stuff (compound types)
3d3dc8
2) check if there's a return between the variable declaration and its
3d3dc8
   initialization; if so, treat the variable as uninitialized
3d3dc8
3) introduction of 2) increased the query runtime exponentially, so
3d3dc8
   introduce some optimizations to bring it back to some reasonable
3d3dc8
   values
3d3dc8
3d3dc8
(cherry picked from commit c8fec8bf9b086f9fc7638db0f1a613a00d7c63a3)
3d3dc8
3d3dc8
Related: #2017033
3d3dc8
---
3d3dc8
 .../UninitializedVariableWithCleanup.ql       | 48 ++++++++++---------
3d3dc8
 1 file changed, 25 insertions(+), 23 deletions(-)
3d3dc8
3d3dc8
diff --git a/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql b/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql
3d3dc8
index 8c24b6d8f1..6b3b62f8bc 100644
3d3dc8
--- a/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql
3d3dc8
+++ b/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql
3d3dc8
@@ -16,24 +16,6 @@
3d3dc8
 import cpp
3d3dc8
 import semmle.code.cpp.controlflow.StackVariableReachability
3d3dc8
 
3d3dc8
-/**
3d3dc8
- * Auxiliary predicate: Types that don't require initialization
3d3dc8
- * before they are used, since they're stack-allocated.
3d3dc8
- */
3d3dc8
-predicate allocatedType(Type t) {
3d3dc8
-  /* Arrays: "int foo[1]; foo[0] = 42;" is ok. */
3d3dc8
-  t instanceof ArrayType
3d3dc8
-  or
3d3dc8
-  /* Structs: "struct foo bar; bar.baz = 42" is ok. */
3d3dc8
-  t instanceof Class
3d3dc8
-  or
3d3dc8
-  /* Typedefs to other allocated types are fine. */
3d3dc8
-  allocatedType(t.(TypedefType).getUnderlyingType())
3d3dc8
-  or
3d3dc8
-  /* Type specifiers don't affect whether or not a type is allocated. */
3d3dc8
-  allocatedType(t.getUnspecifiedType())
3d3dc8
-}
3d3dc8
-
3d3dc8
 /** Auxiliary predicate: List cleanup functions we want to explicitly ignore
3d3dc8
   * since they don't do anything illegal even when the variable is uninitialized
3d3dc8
   */
3d3dc8
@@ -47,13 +29,11 @@ predicate cleanupFunctionDenyList(string fun) {
3d3dc8
  */
3d3dc8
 DeclStmt declWithNoInit(LocalVariable v) {
3d3dc8
   result.getADeclaration() = v and
3d3dc8
-  not exists(v.getInitializer()) and
3d3dc8
+  not v.hasInitializer() and
3d3dc8
   /* The variable has __attribute__((__cleanup__(...))) set */
3d3dc8
   v.getAnAttribute().hasName("cleanup") and
3d3dc8
   /* Check if the cleanup function is not on a deny list */
3d3dc8
-  not exists(Attribute a | a = v.getAnAttribute() and a.getName() = "cleanup" | cleanupFunctionDenyList(a.getAnArgument().getValueText())) and
3d3dc8
-  /* The type of the variable is not stack-allocated. */
3d3dc8
-  exists(Type t | t = v.getType() | not allocatedType(t))
3d3dc8
+  not cleanupFunctionDenyList(v.getAnAttribute().getAnArgument().getValueText())
3d3dc8
 }
3d3dc8
 
3d3dc8
 class UninitialisedLocalReachability extends StackVariableReachability {
3d3dc8
@@ -78,7 +58,29 @@ class UninitialisedLocalReachability extends StackVariableReachability {
3d3dc8
   override predicate isBarrier(ControlFlowNode node, StackVariable v) {
3d3dc8
     // only report the _first_ possibly uninitialized use
3d3dc8
     useOfVar(v, node) or
3d3dc8
-    definitionBarrier(v, node)
3d3dc8
+    (
3d3dc8
+      /* If there's an return statement somewhere between the variable declaration
3d3dc8
+       * and a possible definition, don't accept is as a valid initialization.
3d3dc8
+       *
3d3dc8
+       * E.g.:
3d3dc8
+       * _cleanup_free_ char *x;
3d3dc8
+       * ...
3d3dc8
+       * if (...)
3d3dc8
+       *    return;
3d3dc8
+       * ...
3d3dc8
+       * x = malloc(...);
3d3dc8
+       *
3d3dc8
+       * is not a valid initialization, since we might return from the function
3d3dc8
+       * _before_ the actual iniitialization (emphasis on _might_, since we
3d3dc8
+       * don't know if the return statement might ever evaluate to true).
3d3dc8
+       */
3d3dc8
+      definitionBarrier(v, node) and
3d3dc8
+      not exists(ReturnStmt rs |
3d3dc8
+                 /* The attribute check is "just" a complexity optimization */
3d3dc8
+                 v.getFunction() = rs.getEnclosingFunction() and v.getAnAttribute().hasName("cleanup") |
3d3dc8
+                 rs.getLocation().isBefore(node.getLocation())
3d3dc8
+      )
3d3dc8
+    )
3d3dc8
   }
3d3dc8
 }
3d3dc8