[clang] [clang][StaticAnalyzer] Adding getentropy to CStringChecker. (PR #83675)

David CARLIER via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 8 07:12:11 PST 2024


https://github.com/devnexen updated https://github.com/llvm/llvm-project/pull/83675

>From 1b2fec2c9a41be4ad216d7032189f561eed3f751 Mon Sep 17 00:00:00 2001
From: David Carlier <devnexen at gmail.com>
Date: Sat, 2 Mar 2024 14:56:15 +0000
Subject: [PATCH 1/3] [clang][StaticAnalyzer] Adding getentropy to
 CStringChecker.

since it went way beyond just openbsd, adding basic check for possible
misusage.
---
 .../Checkers/CStringChecker.cpp               | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index 59be236ca1c769..cea99fad3e8436 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -165,6 +165,7 @@ class CStringChecker : public Checker< eval::Call,
       {{CDM::CLibrary, {"explicit_bzero"}, 2}, &CStringChecker::evalBzero},
       {{CDM::CLibrary, {"sprintf"}, 2}, &CStringChecker::evalSprintf},
       {{CDM::CLibrary, {"snprintf"}, 2}, &CStringChecker::evalSnprintf},
+      {{CDM::CLibrary, {"getentropy"}, 2}, &CStringChecker::evalGetentropy},
   };
 
   // These require a bit of special handling.
@@ -219,6 +220,7 @@ class CStringChecker : public Checker< eval::Call,
   void evalSnprintf(CheckerContext &C, const CallEvent &Call) const;
   void evalSprintfCommon(CheckerContext &C, const CallEvent &Call,
                          bool IsBounded, bool IsBuiltin) const;
+  void evalGetentropy(CheckerContext &C, const CallEvent &Call) const;
 
   // Utility methods
   std::pair<ProgramStateRef , ProgramStateRef >
@@ -2515,6 +2517,47 @@ void CStringChecker::evalSprintfCommon(CheckerContext &C, const CallEvent &Call,
   C.addTransition(State);
 }
 
+void CStringChecker::evalGetentropy(CheckerContext &C,
+                                    const CallEvent &Call) const {
+  DestinationArgExpr Buffer = {{Call.getArgExpr(0), 0}};
+  SizeArgExpr Size = {{Call.getArgExpr(1), 1}};
+  ProgramStateRef State = C.getState();
+  constexpr int BufferMaxSize = 256;
+
+  SVal SizeVal = C.getSVal(Size.Expression);
+  QualType SizeTy = Size.Expression->getType();
+
+  ProgramStateRef StateZeroSize, StateNonZeroSize;
+  std::tie(StateZeroSize, StateNonZeroSize) =
+      assumeZero(C, State, SizeVal, SizeTy);
+
+  SVal Buff = C.getSVal(Buffer.Expression);
+  State = checkNonNull(C, StateNonZeroSize, Buffer, Buff);
+  if (!State)
+    return;
+
+  State = CheckBufferAccess(C, State, Buffer, Size, AccessKind::write);
+  if (!State)
+    return;
+
+  auto SizeLoc = SizeVal.getAs<nonloc::ConcreteInt>();
+  auto size = SizeLoc->getValue().getExtValue();
+
+  if (size > BufferMaxSize) {
+    ErrorMessage Message;
+    llvm::raw_svector_ostream Os(Message);
+    Os << " destination buffer size is greater than " << BufferMaxSize;
+    emitOutOfBoundsBug(C, StateNonZeroSize, Buffer.Expression, Message);
+    return;
+  }
+
+  State = invalidateDestinationBufferBySize(C, State, Buffer.Expression,
+                                            C.getSVal(Buffer.Expression),
+                                            SizeVal, SizeTy);
+
+  C.addTransition(State);
+}
+
 //===----------------------------------------------------------------------===//
 // The driver method, and other Checker callbacks.
 //===----------------------------------------------------------------------===//

>From 4c626fa147aade7725e04dc633b53aefcd1347b0 Mon Sep 17 00:00:00 2001
From: David Carlier <devnexen at gmail.com>
Date: Wed, 6 Mar 2024 17:38:25 +0000
Subject: [PATCH 2/3] few fixes and tests additions

---
 .../Checkers/CStringChecker.cpp               | 51 +++++++++++--------
 clang/test/Analysis/bstring.c                 | 39 ++++++++++++++
 2 files changed, 70 insertions(+), 20 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index cea99fad3e8436..4d0492bcaf159e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -165,7 +165,8 @@ class CStringChecker : public Checker< eval::Call,
       {{CDM::CLibrary, {"explicit_bzero"}, 2}, &CStringChecker::evalBzero},
       {{CDM::CLibrary, {"sprintf"}, 2}, &CStringChecker::evalSprintf},
       {{CDM::CLibrary, {"snprintf"}, 2}, &CStringChecker::evalSnprintf},
-      {{CDM::CLibrary, {"getentropy"}, 2}, &CStringChecker::evalGetentropy},
+      {{CDM::CLibrary, {"getentropy"}, 2},
+       std::bind(&CStringChecker::evalGetentropy, _1, _2, _3, CK_Regular)},
   };
 
   // These require a bit of special handling.
@@ -220,7 +221,7 @@ class CStringChecker : public Checker< eval::Call,
   void evalSnprintf(CheckerContext &C, const CallEvent &Call) const;
   void evalSprintfCommon(CheckerContext &C, const CallEvent &Call,
                          bool IsBounded, bool IsBuiltin) const;
-  void evalGetentropy(CheckerContext &C, const CallEvent &Call) const;
+  void evalGetentropy(CheckerContext &C, const CallEvent &Call, CharKind CK) const;
 
   // Utility methods
   std::pair<ProgramStateRef , ProgramStateRef >
@@ -2518,11 +2519,13 @@ void CStringChecker::evalSprintfCommon(CheckerContext &C, const CallEvent &Call,
 }
 
 void CStringChecker::evalGetentropy(CheckerContext &C,
-                                    const CallEvent &Call) const {
+                                    const CallEvent &Call, CharKind CK) const {
   DestinationArgExpr Buffer = {{Call.getArgExpr(0), 0}};
   SizeArgExpr Size = {{Call.getArgExpr(1), 1}};
   ProgramStateRef State = C.getState();
-  constexpr int BufferMaxSize = 256;
+  const LocationContext *LCtx = C.getLocationContext();
+  SValBuilder &Builder = C.getSValBuilder();
+  SVal MaxLength = Builder.makeIntVal(256, C.getASTContext().IntTy);
 
   SVal SizeVal = C.getSVal(Size.Expression);
   QualType SizeTy = Size.Expression->getType();
@@ -2531,31 +2534,39 @@ void CStringChecker::evalGetentropy(CheckerContext &C,
   std::tie(StateZeroSize, StateNonZeroSize) =
       assumeZero(C, State, SizeVal, SizeTy);
 
-  SVal Buff = C.getSVal(Buffer.Expression);
-  State = checkNonNull(C, StateNonZeroSize, Buffer, Buff);
-  if (!State)
+  if (StateZeroSize) {
+    StateZeroSize = State->BindExpr(Call.getOriginExpr(), LCtx,
+                              Builder.makeZeroVal(C.getASTContext().IntTy));
+    C.addTransition(StateZeroSize);
     return;
+  }
 
-  State = CheckBufferAccess(C, State, Buffer, Size, AccessKind::write);
+  SVal Buff = C.getSVal(Buffer.Expression);
+  State = checkNonNull(C, StateNonZeroSize, Buffer, Buff);
   if (!State)
     return;
 
-  auto SizeLoc = SizeVal.getAs<nonloc::ConcreteInt>();
-  auto size = SizeLoc->getValue().getExtValue();
-
-  if (size > BufferMaxSize) {
+  QualType cmpTy = C.getSValBuilder().getConditionType();
+  ProgramStateRef bufferTooLong, bufferNotTooLong;
+  std::tie(bufferTooLong, bufferNotTooLong) = State->assume(
+	 Builder
+	.evalBinOpNN(State, BO_GT, *SizeVal.getAs<NonLoc>(), *MaxLength.getAs<NonLoc>(), cmpTy)
+	.castAs<DefinedOrUnknownSVal>());
+  if (bufferTooLong) {
     ErrorMessage Message;
     llvm::raw_svector_ostream Os(Message);
-    Os << " destination buffer size is greater than " << BufferMaxSize;
-    emitOutOfBoundsBug(C, StateNonZeroSize, Buffer.Expression, Message);
-    return;
-  }
+    Os << "size is greater than 256";
+    emitOutOfBoundsBug(C, bufferTooLong, Buffer.Expression, Message);
+  } else {
+    State = CheckBufferAccess(C, State, Buffer, Size, AccessKind::write, CK);
+    if (!State)
+      return;
 
-  State = invalidateDestinationBufferBySize(C, State, Buffer.Expression,
-                                            C.getSVal(Buffer.Expression),
+    State = invalidateDestinationBufferBySize(C, State, Buffer.Expression,
+                                            Buff,
                                             SizeVal, SizeTy);
-
-  C.addTransition(State);
+    C.addTransition(State);
+  }
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/clang/test/Analysis/bstring.c b/clang/test/Analysis/bstring.c
index f015e0b5d9fb7b..0696e98f8417dd 100644
--- a/clang/test/Analysis/bstring.c
+++ b/clang/test/Analysis/bstring.c
@@ -529,3 +529,42 @@ void nocrash_on_locint_offset(void *addr, void* from, struct S s) {
   size_t iAdd = (size_t) addr;
   memcpy(((void *) &(s.f)), from, iAdd);
 }
+
+#if defined(__linux__) || defined(__FreeBSD__) || defined(__OpenBSD__)
+/* present in both glibc 2.25 and musl 1.1.20 */
+
+//===----------------------------------------------------------------------===
+// getentropy()
+//===----------------------------------------------------------------------===
+
+int getentropy(void *d, size_t n);
+
+int getentropy0(void) {
+  char buf[16] = {0};
+
+  int r = getentropy(buf, sizeof(buf)); // no-warning
+  return r;
+}
+
+int getentropy1(void) {
+  char buf[257] = {0};
+
+  int r = getentropy(buf, 256); // no-warning
+  return r;
+}
+
+int getentropy2(void) {
+  char buf[1024] = {0};
+
+  int r = getentropy(buf, sizeof(buf)); // expected-warning{{size is greater than 256}}
+  return r;
+}
+
+int getentropy3(void) {
+  char buf[256] = {0};
+
+  int r = getentropy(buf, 0); // no-wwarning
+  return r;
+}
+
+#endif

>From f10b99a978b6833e24eb098898d71bc54217b67d Mon Sep 17 00:00:00 2001
From: David Carlier <devnexen at gmail.com>
Date: Fri, 8 Mar 2024 15:11:59 +0000
Subject: [PATCH 3/3] further changes from feedback

---
 .../Checkers/CStringChecker.cpp               | 31 ++++++++-----------
 clang/test/Analysis/bstring.c                 | 11 ++-----
 2 files changed, 16 insertions(+), 26 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index 4d0492bcaf159e..45c1d51c71d2c8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -165,8 +165,7 @@ class CStringChecker : public Checker< eval::Call,
       {{CDM::CLibrary, {"explicit_bzero"}, 2}, &CStringChecker::evalBzero},
       {{CDM::CLibrary, {"sprintf"}, 2}, &CStringChecker::evalSprintf},
       {{CDM::CLibrary, {"snprintf"}, 2}, &CStringChecker::evalSnprintf},
-      {{CDM::CLibrary, {"getentropy"}, 2},
-       std::bind(&CStringChecker::evalGetentropy, _1, _2, _3, CK_Regular)},
+      {{CDM::CLibrary, {"getentropy"}, 2}, &CStringChecker::evalGetentropy},
   };
 
   // These require a bit of special handling.
@@ -221,7 +220,7 @@ class CStringChecker : public Checker< eval::Call,
   void evalSnprintf(CheckerContext &C, const CallEvent &Call) const;
   void evalSprintfCommon(CheckerContext &C, const CallEvent &Call,
                          bool IsBounded, bool IsBuiltin) const;
-  void evalGetentropy(CheckerContext &C, const CallEvent &Call, CharKind CK) const;
+  void evalGetentropy(CheckerContext &C, const CallEvent &Call) const;
 
   // Utility methods
   std::pair<ProgramStateRef , ProgramStateRef >
@@ -2518,14 +2517,12 @@ void CStringChecker::evalSprintfCommon(CheckerContext &C, const CallEvent &Call,
   C.addTransition(State);
 }
 
-void CStringChecker::evalGetentropy(CheckerContext &C,
-                                    const CallEvent &Call, CharKind CK) const {
+void CStringChecker::evalGetentropy(CheckerContext &C, const CallEvent &Call) const {
   DestinationArgExpr Buffer = {{Call.getArgExpr(0), 0}};
   SizeArgExpr Size = {{Call.getArgExpr(1), 1}};
   ProgramStateRef State = C.getState();
-  const LocationContext *LCtx = C.getLocationContext();
-  SValBuilder &Builder = C.getSValBuilder();
-  SVal MaxLength = Builder.makeIntVal(256, C.getASTContext().IntTy);
+  SValBuilder &SVB = C.getSValBuilder();
+  SVal MaxLength = SVB.makeIntVal(256, C.getASTContext().IntTy);
 
   SVal SizeVal = C.getSVal(Size.Expression);
   QualType SizeTy = Size.Expression->getType();
@@ -2535,8 +2532,8 @@ void CStringChecker::evalGetentropy(CheckerContext &C,
       assumeZero(C, State, SizeVal, SizeTy);
 
   if (StateZeroSize) {
-    StateZeroSize = State->BindExpr(Call.getOriginExpr(), LCtx,
-                              Builder.makeZeroVal(C.getASTContext().IntTy));
+    StateZeroSize = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
+                              SVB.makeZeroVal(C.getASTContext().IntTy));
     C.addTransition(StateZeroSize);
     return;
   }
@@ -2547,18 +2544,16 @@ void CStringChecker::evalGetentropy(CheckerContext &C,
     return;
 
   QualType cmpTy = C.getSValBuilder().getConditionType();
-  ProgramStateRef bufferTooLong, bufferNotTooLong;
-  std::tie(bufferTooLong, bufferNotTooLong) = State->assume(
-	 Builder
+  ProgramStateRef sizeAboveLimit, sizeNotAboveLimit;
+  std::tie(sizeAboveLimit, sizeNotAboveLimit) = State->assume(
+	 SVB
 	.evalBinOpNN(State, BO_GT, *SizeVal.getAs<NonLoc>(), *MaxLength.getAs<NonLoc>(), cmpTy)
 	.castAs<DefinedOrUnknownSVal>());
-  if (bufferTooLong) {
+  if (sizeAboveLimit) {
     ErrorMessage Message;
-    llvm::raw_svector_ostream Os(Message);
-    Os << "size is greater than 256";
-    emitOutOfBoundsBug(C, bufferTooLong, Buffer.Expression, Message);
+    emitOutOfBoundsBug(C, sizeAboveLimit, Buffer.Expression, "must be smaller than or equal to 256");
   } else {
-    State = CheckBufferAccess(C, State, Buffer, Size, AccessKind::write, CK);
+    State = CheckBufferAccess(C, State, Buffer, Size, AccessKind::write);
     if (!State)
       return;
 
diff --git a/clang/test/Analysis/bstring.c b/clang/test/Analysis/bstring.c
index 0696e98f8417dd..10ceb27690909f 100644
--- a/clang/test/Analysis/bstring.c
+++ b/clang/test/Analysis/bstring.c
@@ -530,12 +530,9 @@ void nocrash_on_locint_offset(void *addr, void* from, struct S s) {
   memcpy(((void *) &(s.f)), from, iAdd);
 }
 
-#if defined(__linux__) || defined(__FreeBSD__) || defined(__OpenBSD__)
-/* present in both glibc 2.25 and musl 1.1.20 */
-
-//===----------------------------------------------------------------------===
+//===----------------------------------------------------------------------===//
 // getentropy()
-//===----------------------------------------------------------------------===
+//===----------------------------------------------------------------------===//
 
 int getentropy(void *d, size_t n);
 
@@ -556,7 +553,7 @@ int getentropy1(void) {
 int getentropy2(void) {
   char buf[1024] = {0};
 
-  int r = getentropy(buf, sizeof(buf)); // expected-warning{{size is greater than 256}}
+  int r = getentropy(buf, sizeof(buf)); // expected-warning{{must be smaller than or equal to 256}}
   return r;
 }
 
@@ -566,5 +563,3 @@ int getentropy3(void) {
   int r = getentropy(buf, 0); // no-wwarning
   return r;
 }
-
-#endif



More information about the cfe-commits mailing list