From cecb3cc06f6025835324c1837c03def1d9be8eb1 Mon Sep 17 00:00:00 2001

From: Frantisek Sumsal <frantisek@sumsal.cz>

Date: Wed, 1 Dec 2021 21:31:43 +0100

Subject: [PATCH] lgtm: detect uninitialized variables using the __cleanup__

 attribute

This is a slightly modified version of the original

`cpp/uninitialized-local` CodeQL query which focuses only on variables

using the cleanup macros. Since this has proven to cause issues in the

past, let's panic on every uninitialized variable using any of the

cleanup macros (as long as they're written using the __cleanup__

attribute).

Some test results from a test I used when writing the query:

```

 #define _cleanup_foo_ __attribute__((__cleanup__(foo)))

 #define _cleanup_(x) __attribute__((__cleanup__(x)))

 static inline void freep(void *p) {

         *(void**)p = mfree(*(void**) p);

 }

 #define _cleanup_free_ _cleanup_(freep)

 static inline void foo(char **p) {

     if (*p)

         *p = free(*p);

 }

 int main(void) {

     __attribute__((__cleanup__(foo))) char *a;

     char *b;

     _cleanup_foo_ char *c;

     char **d;

     _cleanup_free_ char *e;

     int r;

     r = fun(&e);

     if (r < 0)

         return 1;

     puts(a);

     puts(b);

     puts(c);

     puts(*d);

     puts(e);

     return 0;

 }

```

```

+| test.c:23:14:23:14 | e | The variable $@ may not be initialized here, but has a cleanup handler. | test.c:20:26:20:26 | e | e |

+| test.c:27:10:27:10 | a | The variable $@ may not be initialized here, but has a cleanup handler. | test.c:16:45:16:45 | a | a |

+| test.c:29:10:29:10 | c | The variable $@ may not be initialized here, but has a cleanup handler. | test.c:18:25:18:25 | c | c |

```

(cherry picked from commit 863bff75488d33f519deea6390988f3d9d72e6de)

Related: #2017033

---

 .../UninitializedVariableWithCleanup.ql       | 99 +++++++++++++++++++

 1 file changed, 99 insertions(+)

 create mode 100644 .lgtm/cpp-queries/UninitializedVariableWithCleanup.ql

diff --git a/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql b/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql

new file mode 100644

index 0000000000..6bf0ae01eb

--- /dev/null

+++ b/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql

@@ -0,0 +1,99 @@

+/**

+ * vi: sw=2 ts=2 et syntax=ql:

+ *

+ * Based on cpp/uninitialized-local.

+ *

+ * @name Potentially uninitialized local variable using the cleanup attribute

+ * @description Running the cleanup handler on a possibly uninitialized variable

+ *              is generally a bad idea.

+ * @id cpp/uninitialized-local-with-cleanup

+ * @kind problem

+ * @problem.severity error

+ * @precision high

+ * @tags security

+ */

+

+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())

+}

+

+/**

+ * A declaration of a local variable using __attribute__((__cleanup__(x)))

+ * that leaves the variable uninitialized.

+ */

+DeclStmt declWithNoInit(LocalVariable v) {

+  result.getADeclaration() = v and

+  not exists(v.getInitializer()) and

+  /* The variable has __attribute__((__cleanup__(...))) set */

+  v.getAnAttribute().hasName("cleanup") and

+  /* The type of the variable is not stack-allocated. */

+  exists(Type t | t = v.getType() | not allocatedType(t))

+}

+

+class UninitialisedLocalReachability extends StackVariableReachability {

+  UninitialisedLocalReachability() { this = "UninitialisedLocal" }

+

+  override predicate isSource(ControlFlowNode node, StackVariable v) { node = declWithNoInit(v) }

+

+  /* Note: _don't_ use the `useOfVarActual()` predicate here (and a couple of lines

+   * below), as it assumes that the callee always modifies the variable if

+   * it's passed to the function.

+   *

+   * i.e.:

+   * _cleanup_free char *x;

+   * fun(&x);

+   * puts(x);

+   *

+   * `useOfVarActual()` won't treat this an an uninitialized read even if the callee

+   * doesn't modify the argument, however, `useOfVar()` will

+   */

+  override predicate isSink(ControlFlowNode node, StackVariable v) { useOfVar(v, node) }

+

+  override predicate isBarrier(ControlFlowNode node, StackVariable v) {

+    // only report the _first_ possibly uninitialized use

+    useOfVar(v, node) or

+    definitionBarrier(v, node)

+  }

+}

+

+pragma[noinline]

+predicate containsInlineAssembly(Function f) { exists(AsmStmt s | s.getEnclosingFunction() = f) }

+

+/**

+ * Auxiliary predicate: List common exceptions or false positives

+ * for this check to exclude them.

+ */

+VariableAccess commonException() {

+  // If the uninitialized use we've found is in a macro expansion, it's

+  // typically something like va_start(), and we don't want to complain.

+  result.getParent().isInMacroExpansion()

+  or

+  result.getParent() instanceof BuiltInOperation

+  or

+  // Finally, exclude functions that contain assembly blocks. It's

+  // anyone's guess what happens in those.

+  containsInlineAssembly(result.getEnclosingFunction())

+}

+

+from UninitialisedLocalReachability r, LocalVariable v, VariableAccess va

+where

+  r.reaches(_, v, va) and

+  not va = commonException()

+select va, "The variable $@ may not be initialized here, but has a cleanup handler.", v, v.getName()