[clang] [clang][analyzer] Model allocation behavior or getdelim/geline (PR #83138)

Alejandro Álvarez Ayllón via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 27 06:33:46 PST 2024


https://github.com/alejandro-alvarez-sonarsource updated https://github.com/llvm/llvm-project/pull/83138

>From ad17cfb588500d095b4e402bd2f2197772f89b12 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
 <alejandro.alvarez at sonarsource.com>
Date: Wed, 21 Feb 2024 16:08:04 +0100
Subject: [PATCH 1/2] [clang][analyzer] Model allocation behavior or
 getdelim/geline

---
 .../Core/PathSensitive/CheckerHelpers.h       |  6 ++
 .../StaticAnalyzer/Checkers/MallocChecker.cpp | 73 +++++++++++++++++--
 .../StaticAnalyzer/Core/CheckerHelpers.cpp    |  9 +++
 .../system-header-simulator-for-malloc.h      |  1 +
 clang/test/Analysis/getline-alloc.c           | 71 ++++++++++++++++++
 5 files changed, 154 insertions(+), 6 deletions(-)
 create mode 100644 clang/test/Analysis/getline-alloc.c

diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
index 65982457ad8393..60421e5437d82f 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
@@ -13,6 +13,9 @@
 #ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CHECKERHELPERS_H
 #define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CHECKERHELPERS_H
 
+#include "ProgramState_Fwd.h"
+#include "SVals.h"
+
 #include "clang/AST/OperationKinds.h"
 #include "clang/AST/Stmt.h"
 #include "clang/Basic/OperatorKinds.h"
@@ -110,6 +113,9 @@ class OperatorKind {
 OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
                                                  bool IsBinary);
 
+std::optional<DefinedSVal> getPointeeDefVal(SVal PtrSVal,
+                                            ProgramStateRef State);
+
 } // namespace ento
 
 } // namespace clang
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 79ab05f2c7866a..c5c382634c981c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -382,6 +382,8 @@ class MallocChecker
   CHECK_FN(checkGMemdup)
   CHECK_FN(checkGMallocN)
   CHECK_FN(checkGMallocN0)
+  CHECK_FN(preGetdelim)
+  CHECK_FN(checkGetdelim)
   CHECK_FN(checkReallocN)
   CHECK_FN(checkOwnershipAttr)
 
@@ -391,6 +393,11 @@ class MallocChecker
   using CheckFn = std::function<void(const MallocChecker *,
                                      const CallEvent &Call, CheckerContext &C)>;
 
+  const CallDescriptionMap<CheckFn> PreFnMap{
+      {{{"getline"}, 3}, &MallocChecker::preGetdelim},
+      {{{"getdelim"}, 4}, &MallocChecker::preGetdelim},
+  };
+
   const CallDescriptionMap<CheckFn> FreeingMemFnMap{
       {{{"free"}, 1}, &MallocChecker::checkFree},
       {{{"if_freenameindex"}, 1}, &MallocChecker::checkIfFreeNameIndex},
@@ -439,6 +446,8 @@ class MallocChecker
        std::bind(&MallocChecker::checkRealloc, _1, _2, _3, false)},
       {{{"g_realloc_n"}, 3}, &MallocChecker::checkReallocN},
       {{{"g_try_realloc_n"}, 3}, &MallocChecker::checkReallocN},
+      {{{"getline"}, 3}, &MallocChecker::checkGetdelim},
+      {{{"getdelim"}, 4}, &MallocChecker::checkGetdelim},
   };
 
   bool isMemCall(const CallEvent &Call) const;
@@ -588,11 +597,14 @@ class MallocChecker
   ///      }
   /// \param [in] ReturnsNullOnFailure Whether the memory deallocation function
   ///   we're modeling returns with Null on failure.
+  /// \param [in] ArgValOpt Optional value to use for the argument instead of
+  /// the one obtained from ArgExpr.
   /// \returns The ProgramState right after deallocation.
   [[nodiscard]] ProgramStateRef
   FreeMemAux(CheckerContext &C, const Expr *ArgExpr, const CallEvent &Call,
              ProgramStateRef State, bool Hold, bool &IsKnownToBeAllocated,
-             AllocationFamily Family, bool ReturnsNullOnFailure = false) const;
+             AllocationFamily Family, bool ReturnsNullOnFailure = false,
+             std::optional<SVal> ArgValOpt = {}) const;
 
   // TODO: Needs some refactoring, as all other deallocation modeling
   // functions are suffering from out parameters and messy code due to how
@@ -1423,6 +1435,46 @@ void MallocChecker::checkGMallocN0(const CallEvent &Call,
   C.addTransition(State);
 }
 
+void MallocChecker::preGetdelim(const CallEvent &Call,
+                                CheckerContext &C) const {
+  if (!Call.isGlobalCFunction())
+    return;
+
+  ProgramStateRef State = C.getState();
+  const auto LinePtr = getPointeeDefVal(Call.getArgSVal(0), State);
+  if (!LinePtr)
+    return;
+
+  bool IsKnownToBeAllocated = false;
+  State = FreeMemAux(C, Call.getArgExpr(0), Call, State, false,
+                     IsKnownToBeAllocated, AF_Malloc, false, LinePtr);
+  if (State)
+    C.addTransition(State);
+}
+
+void MallocChecker::checkGetdelim(const CallEvent &Call,
+                                  CheckerContext &C) const {
+  if (!Call.isGlobalCFunction())
+    return;
+
+  ProgramStateRef State = C.getState();
+  // Handle the post-conditions of getline and getdelim:
+  // Register the new conjured value as an allocated buffer.
+  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return;
+
+  SValBuilder &svalBuilder = C.getSValBuilder();
+
+  const auto LinePtr = getPointeeDefVal(Call.getArgSVal(0), State);
+  const auto Size = getPointeeDefVal(Call.getArgSVal(1), State);
+  if (!LinePtr || !Size || !LinePtr->getAsRegion())
+    return;
+
+  State = setDynamicExtent(State, LinePtr->getAsRegion(), *Size, svalBuilder);
+  C.addTransition(MallocUpdateRefState(C, CE, State, AF_Malloc, *LinePtr));
+}
+
 void MallocChecker::checkReallocN(const CallEvent &Call,
                                   CheckerContext &C) const {
   ProgramStateRef State = C.getState();
@@ -1895,15 +1947,17 @@ static void printExpectedDeallocName(raw_ostream &os, AllocationFamily Family) {
   }
 }
 
-ProgramStateRef MallocChecker::FreeMemAux(
-    CheckerContext &C, const Expr *ArgExpr, const CallEvent &Call,
-    ProgramStateRef State, bool Hold, bool &IsKnownToBeAllocated,
-    AllocationFamily Family, bool ReturnsNullOnFailure) const {
+ProgramStateRef
+MallocChecker::FreeMemAux(CheckerContext &C, const Expr *ArgExpr,
+                          const CallEvent &Call, ProgramStateRef State,
+                          bool Hold, bool &IsKnownToBeAllocated,
+                          AllocationFamily Family, bool ReturnsNullOnFailure,
+                          std::optional<SVal> ArgValOpt) const {
 
   if (!State)
     return nullptr;
 
-  SVal ArgVal = C.getSVal(ArgExpr);
+  SVal ArgVal = ArgValOpt.value_or(C.getSVal(ArgExpr));
   if (!isa<DefinedOrUnknownSVal>(ArgVal))
     return nullptr;
   DefinedOrUnknownSVal location = ArgVal.castAs<DefinedOrUnknownSVal>();
@@ -2881,6 +2935,13 @@ void MallocChecker::checkPreCall(const CallEvent &Call,
       return;
   }
 
+  // We need to handle getline pre-conditions here before the pointed region
+  // gets invalidated by StreamChecker
+  if (const auto *PreFN = PreFnMap.lookup(Call)) {
+    (*PreFN)(this, Call, C);
+    return;
+  }
+
   // We will check for double free in the post visit.
   if (const AnyFunctionCall *FC = dyn_cast<AnyFunctionCall>(&Call)) {
     const FunctionDecl *FD = FC->getDecl();
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp b/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
index 84ad20a5480792..364c87e910b7b5 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -14,6 +14,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
 #include "clang/Lex/Preprocessor.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include <optional>
 
 namespace clang {
@@ -182,5 +183,13 @@ OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
   }
 }
 
+std::optional<DefinedSVal> getPointeeDefVal(SVal PtrSVal,
+                                            ProgramStateRef State) {
+  if (const auto *Ptr = PtrSVal.getAsRegion()) {
+    return State->getSVal(Ptr).getAs<DefinedSVal>();
+  }
+  return std::nullopt;
+}
+
 } // namespace ento
 } // namespace clang
diff --git a/clang/test/Analysis/Inputs/system-header-simulator-for-malloc.h b/clang/test/Analysis/Inputs/system-header-simulator-for-malloc.h
index e76455655e9e2e..bc7009eb0d1bea 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator-for-malloc.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator-for-malloc.h
@@ -9,6 +9,7 @@ typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void *calloc(size_t, size_t);
 void free(void *);
+void *alloca(size_t);
 
 
 #if __OBJC__
diff --git a/clang/test/Analysis/getline-alloc.c b/clang/test/Analysis/getline-alloc.c
new file mode 100644
index 00000000000000..eb2002af1219d1
--- /dev/null
+++ b/clang/test/Analysis/getline-alloc.c
@@ -0,0 +1,71 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection -verify %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,alpha.unix,debug.ExprInspection -verify %s
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/system-header-simulator-for-malloc.h"
+
+void test_getline_null_buffer() {
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return;
+  char *buffer = NULL;
+  size_t n = 0;
+  if (getline(&buffer, &n, F1) > 0) {
+    char c = buffer[0]; // ok
+  }
+  free(buffer);
+  fclose(F1);
+}
+
+void test_getline_malloc_buffer() {
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return;
+
+  size_t n = 10;
+  char *buffer = malloc(n);
+  char *ptr = buffer;
+
+  ssize_t r = getdelim(&buffer, &n, '\r', F1);
+  // ptr may be dangling
+  free(ptr);    // expected-warning {{Attempt to free released memory}}
+  free(buffer); // ok
+  fclose(F1);
+}
+
+void test_getline_alloca() {
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return;
+  size_t n = 10;
+  char *buffer = alloca(n);
+  getline(&buffer, &n, F1); // expected-warning {{Memory allocated by alloca() should not be deallocated}}
+  fclose(F1);
+}
+
+void test_getline_invalid_ptr() {
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return;
+  size_t n = 10;
+  char *buffer = (char*)test_getline_invalid_ptr;
+  getline(&buffer, &n, F1); // expected-warning {{Argument to getline() is the address of the function 'test_getline_invalid_ptr', which is not memory allocated by malloc()}}
+  fclose(F1);
+}
+
+void test_getline_leak() {
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return;
+
+  char *buffer = NULL;
+  size_t n = 0;
+  ssize_t read;
+
+  while ((read = getline(&buffer, &n, F1)) != -1) {
+    printf("%s\n", buffer);
+  }
+
+  fclose(F1); // expected-warning {{Potential memory leak}}
+}

>From 42b1e801bdd155392d01f77bd32b1004f6be0e8c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
 <alejandro.alvarez at sonarsource.com>
Date: Tue, 27 Feb 2024 15:33:13 +0100
Subject: [PATCH 2/2] Add two more test cases

---
 clang/test/Analysis/getline-alloc.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/clang/test/Analysis/getline-alloc.c b/clang/test/Analysis/getline-alloc.c
index eb2002af1219d1..5b5c716cb605a7 100644
--- a/clang/test/Analysis/getline-alloc.c
+++ b/clang/test/Analysis/getline-alloc.c
@@ -69,3 +69,27 @@ void test_getline_leak() {
 
   fclose(F1); // expected-warning {{Potential memory leak}}
 }
+
+void test_getline_stack() {
+  size_t n = 10;
+  char buffer[10];
+  char *ptr = buffer;
+
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return;
+
+  getline(&ptr, &n, F1); // expected-warning {{Argument to getline() is the address of the local variable 'buffer', which is not memory allocated by malloc()}}
+}
+
+void test_getline_static() {
+  static size_t n = 10;
+  static char buffer[10];
+  char *ptr = buffer;
+
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return;
+
+  getline(&ptr, &n, F1); // expected-warning {{Argument to getline() is the address of the static variable 'buffer', which is not memory allocated by malloc()}}
+}



More information about the cfe-commits mailing list