daandemeyer / rpms / systemd

Forked from rpms/systemd 2 years ago
Clone
3d3dc8
From cecb3cc06f6025835324c1837c03def1d9be8eb1 Mon Sep 17 00:00:00 2001
3d3dc8
From: Frantisek Sumsal <frantisek@sumsal.cz>
3d3dc8
Date: Wed, 1 Dec 2021 21:31:43 +0100
3d3dc8
Subject: [PATCH] lgtm: detect uninitialized variables using the __cleanup__
3d3dc8
 attribute
3d3dc8
3d3dc8
This is a slightly modified version of the original
3d3dc8
`cpp/uninitialized-local` CodeQL query which focuses only on variables
3d3dc8
using the cleanup macros. Since this has proven to cause issues in the
3d3dc8
past, let's panic on every uninitialized variable using any of the
3d3dc8
cleanup macros (as long as they're written using the __cleanup__
3d3dc8
attribute).
3d3dc8
3d3dc8
Some test results from a test I used when writing the query:
3d3dc8
3d3dc8
```
3d3dc8
 #define _cleanup_foo_ __attribute__((__cleanup__(foo)))
3d3dc8
 #define _cleanup_(x) __attribute__((__cleanup__(x)))
3d3dc8
3d3dc8
 static inline void freep(void *p) {
3d3dc8
         *(void**)p = mfree(*(void**) p);
3d3dc8
 }
3d3dc8
3d3dc8
 #define _cleanup_free_ _cleanup_(freep)
3d3dc8
3d3dc8
 static inline void foo(char **p) {
3d3dc8
     if (*p)
3d3dc8
         *p = free(*p);
3d3dc8
 }
3d3dc8
3d3dc8
 int main(void) {
3d3dc8
     __attribute__((__cleanup__(foo))) char *a;
3d3dc8
     char *b;
3d3dc8
     _cleanup_foo_ char *c;
3d3dc8
     char **d;
3d3dc8
     _cleanup_free_ char *e;
3d3dc8
     int r;
3d3dc8
3d3dc8
     r = fun(&e);
3d3dc8
     if (r < 0)
3d3dc8
         return 1;
3d3dc8
3d3dc8
     puts(a);
3d3dc8
     puts(b);
3d3dc8
     puts(c);
3d3dc8
     puts(*d);
3d3dc8
     puts(e);
3d3dc8
3d3dc8
     return 0;
3d3dc8
 }
3d3dc8
```
3d3dc8
3d3dc8
```
3d3dc8
+| 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 |
3d3dc8
+| 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 |
3d3dc8
+| 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 |
3d3dc8
```
3d3dc8
3d3dc8
(cherry picked from commit 863bff75488d33f519deea6390988f3d9d72e6de)
3d3dc8
3d3dc8
Related: #2017033
3d3dc8
---
3d3dc8
 .../UninitializedVariableWithCleanup.ql       | 99 +++++++++++++++++++
3d3dc8
 1 file changed, 99 insertions(+)
3d3dc8
 create mode 100644 .lgtm/cpp-queries/UninitializedVariableWithCleanup.ql
3d3dc8
3d3dc8
diff --git a/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql b/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql
3d3dc8
new file mode 100644
3d3dc8
index 0000000000..6bf0ae01eb
3d3dc8
--- /dev/null
3d3dc8
+++ b/.lgtm/cpp-queries/UninitializedVariableWithCleanup.ql
3d3dc8
@@ -0,0 +1,99 @@
3d3dc8
+/**
3d3dc8
+ * vi: sw=2 ts=2 et syntax=ql:
3d3dc8
+ *
3d3dc8
+ * Based on cpp/uninitialized-local.
3d3dc8
+ *
3d3dc8
+ * @name Potentially uninitialized local variable using the cleanup attribute
3d3dc8
+ * @description Running the cleanup handler on a possibly uninitialized variable
3d3dc8
+ *              is generally a bad idea.
3d3dc8
+ * @id cpp/uninitialized-local-with-cleanup
3d3dc8
+ * @kind problem
3d3dc8
+ * @problem.severity error
3d3dc8
+ * @precision high
3d3dc8
+ * @tags security
3d3dc8
+ */
3d3dc8
+
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
+/**
3d3dc8
+ * A declaration of a local variable using __attribute__((__cleanup__(x)))
3d3dc8
+ * that leaves the variable uninitialized.
3d3dc8
+ */
3d3dc8
+DeclStmt declWithNoInit(LocalVariable v) {
3d3dc8
+  result.getADeclaration() = v and
3d3dc8
+  not exists(v.getInitializer()) and
3d3dc8
+  /* The variable has __attribute__((__cleanup__(...))) set */
3d3dc8
+  v.getAnAttribute().hasName("cleanup") and
3d3dc8
+  /* The type of the variable is not stack-allocated. */
3d3dc8
+  exists(Type t | t = v.getType() | not allocatedType(t))
3d3dc8
+}
3d3dc8
+
3d3dc8
+class UninitialisedLocalReachability extends StackVariableReachability {
3d3dc8
+  UninitialisedLocalReachability() { this = "UninitialisedLocal" }
3d3dc8
+
3d3dc8
+  override predicate isSource(ControlFlowNode node, StackVariable v) { node = declWithNoInit(v) }
3d3dc8
+
3d3dc8
+  /* Note: _don't_ use the `useOfVarActual()` predicate here (and a couple of lines
3d3dc8
+   * below), as it assumes that the callee always modifies the variable if
3d3dc8
+   * it's passed to the function.
3d3dc8
+   *
3d3dc8
+   * i.e.:
3d3dc8
+   * _cleanup_free char *x;
3d3dc8
+   * fun(&x);
3d3dc8
+   * puts(x);
3d3dc8
+   *
3d3dc8
+   * `useOfVarActual()` won't treat this an an uninitialized read even if the callee
3d3dc8
+   * doesn't modify the argument, however, `useOfVar()` will
3d3dc8
+   */
3d3dc8
+  override predicate isSink(ControlFlowNode node, StackVariable v) { useOfVar(v, node) }
3d3dc8
+
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
+}
3d3dc8
+
3d3dc8
+pragma[noinline]
3d3dc8
+predicate containsInlineAssembly(Function f) { exists(AsmStmt s | s.getEnclosingFunction() = f) }
3d3dc8
+
3d3dc8
+/**
3d3dc8
+ * Auxiliary predicate: List common exceptions or false positives
3d3dc8
+ * for this check to exclude them.
3d3dc8
+ */
3d3dc8
+VariableAccess commonException() {
3d3dc8
+  // If the uninitialized use we've found is in a macro expansion, it's
3d3dc8
+  // typically something like va_start(), and we don't want to complain.
3d3dc8
+  result.getParent().isInMacroExpansion()
3d3dc8
+  or
3d3dc8
+  result.getParent() instanceof BuiltInOperation
3d3dc8
+  or
3d3dc8
+  // Finally, exclude functions that contain assembly blocks. It's
3d3dc8
+  // anyone's guess what happens in those.
3d3dc8
+  containsInlineAssembly(result.getEnclosingFunction())
3d3dc8
+}
3d3dc8
+
3d3dc8
+from UninitialisedLocalReachability r, LocalVariable v, VariableAccess va
3d3dc8
+where
3d3dc8
+  r.reaches(_, v, va) and
3d3dc8
+  not va = commonException()
3d3dc8
+select va, "The variable $@ may not be initialized here, but has a cleanup handler.", v, v.getName()