[clang] [clang][StaticAnalyzer] Adding getentropy to CStringChecker. (PR #83675)
David CARLIER via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 27 17:12:48 PDT 2024
https://github.com/devnexen updated https://github.com/llvm/llvm-project/pull/83675
>From 010c0c2acddbe36a84382284835e94bffe94b040 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 2866da018b137f2c099f733920a1e15b7e41d289 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 04eefd100f799745fca5923c342d2afb1c75b7bc 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 | 50 ++++++++-----------
clang/test/Analysis/bstring.c | 39 ++++++++++++---
2 files changed, 53 insertions(+), 36 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index 4d0492bcaf159e..2975f2da9c3234 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,53 +2517,48 @@ 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 SizeVal = C.getSVal(Size.Expression);
+ std::optional<NonLoc> SizeVal = C.getSVal(Size.Expression).getAs<NonLoc>();
+ if (!SizeVal)
+ return;
+
+ std::optional<NonLoc> MaxLength = SVB.makeIntVal(256, C.getASTContext().IntTy).getAs<NonLoc>();
QualType SizeTy = Size.Expression->getType();
- ProgramStateRef StateZeroSize, StateNonZeroSize;
- std::tie(StateZeroSize, StateNonZeroSize) =
- assumeZero(C, State, SizeVal, SizeTy);
+ SVal Buff = C.getSVal(Buffer.Expression);
+ auto [StateZeroSize, StateNonZeroSize] =
+ assumeZero(C, State, *SizeVal, SizeTy);
- if (StateZeroSize) {
- StateZeroSize = State->BindExpr(Call.getOriginExpr(), LCtx,
- Builder.makeZeroVal(C.getASTContext().IntTy));
- C.addTransition(StateZeroSize);
+ if (StateZeroSize && !StateNonZeroSize) {
+ State = invalidateDestinationBufferBySize(C, State, Buffer.Expression, Buff, *SizeVal, SizeTy);
+ C.addTransition(State);
return;
}
- SVal Buff = C.getSVal(Buffer.Expression);
State = checkNonNull(C, StateNonZeroSize, Buffer, Buff);
if (!State)
return;
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)
+ auto [sizeAboveLimit, sizeNotAboveLimit] = State->assume(
+ SVB
+ .evalBinOpNN(State, BO_GT, *SizeVal, *MaxLength, cmpTy)
.castAs<DefinedOrUnknownSVal>());
- if (bufferTooLong) {
- ErrorMessage Message;
- llvm::raw_svector_ostream Os(Message);
- Os << "size is greater than 256";
- emitOutOfBoundsBug(C, bufferTooLong, Buffer.Expression, Message);
+ if (sizeAboveLimit && !sizeNotAboveLimit) {
+ 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, sizeNotAboveLimit, Buffer, Size, AccessKind::write);
if (!State)
return;
State = invalidateDestinationBufferBySize(C, State, Buffer.Expression,
Buff,
- SizeVal, SizeTy);
+ *SizeVal, SizeTy);
C.addTransition(State);
}
}
diff --git a/clang/test/Analysis/bstring.c b/clang/test/Analysis/bstring.c
index 0696e98f8417dd..d356cf6573e20e 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,15 +553,41 @@ 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;
}
int getentropy3(void) {
char buf[256] = {0};
- int r = getentropy(buf, 0); // no-wwarning
+ int r = getentropy(buf, 0); // no-warning
return r;
}
-#endif
+int getentropy4(size_t arg) {
+ char buf[256] = {0};
+
+ int r = getentropy(buf, arg); // no-warning
+ return r;
+}
+
+void do_something(void);
+int getentropy5(size_t arg) {
+ char buf[257] = {0};
+
+ // split the state and introduce a separate execution path where arg > 256
+ if (arg <= 256)
+ do_something();
+
+ int r = getentropy(buf, arg); // expected-warning{{must be smaller than or equal to 256}}
+ return r;
+}
+
+int getentropy6(void) {
+ return getentropy(0, 256); // expected-warning{{Null pointer passed as 1st argument to [unix.cstring.NullArg]}}
+}
+
+int getentropy7(void) {
+ char buf[128];
+ return getentropy(buf, sizeof(buf) * 2); // expected-warning{{'getentropy' will always overflow; destination buffer has size 128, but size argument is 256}}
+}
More information about the cfe-commits
mailing list