[clang] [clang][analyzer] Model more getline/getdelim pre and postconditions (PR #83027)
Alejandro Álvarez Ayllón via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 20 04:13:45 PDT 2024
https://github.com/alejandro-alvarez-sonarsource updated https://github.com/llvm/llvm-project/pull/83027
>From a061464b75ac02c21e5d74fc4dff8d8afdbba66b 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, 20 Mar 2024 10:49:08 +0100
Subject: [PATCH 01/18] [NFC] UnixAPIMisuseChecker inherits from
Checker<check::PreCall>
---
.../Checkers/UnixAPIChecker.cpp | 60 +++++++++----------
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
index 19f1ca2dc824c9..599e5e6cedc606 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
@@ -17,6 +17,7 @@
#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
#include "llvm/ADT/STLExtras.h"
@@ -41,8 +42,7 @@ enum class OpenVariant {
namespace {
class UnixAPIMisuseChecker
- : public Checker<check::PreStmt<CallExpr>,
- check::ASTDecl<TranslationUnitDecl>> {
+ : public Checker<check::PreCall, check::ASTDecl<TranslationUnitDecl>> {
const BugType BT_open{this, "Improper use of 'open'", categories::UnixAPI};
const BugType BT_pthreadOnce{this, "Improper use of 'pthread_once'",
categories::UnixAPI};
@@ -52,14 +52,14 @@ class UnixAPIMisuseChecker
void checkASTDecl(const TranslationUnitDecl *TU, AnalysisManager &Mgr,
BugReporter &BR) const;
- void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
+ void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
- void CheckOpen(CheckerContext &C, const CallExpr *CE) const;
- void CheckOpenAt(CheckerContext &C, const CallExpr *CE) const;
- void CheckPthreadOnce(CheckerContext &C, const CallExpr *CE) const;
+ void CheckOpen(CheckerContext &C, const CallEvent &Call) const;
+ void CheckOpenAt(CheckerContext &C, const CallEvent &Call) const;
+ void CheckPthreadOnce(CheckerContext &C, const CallEvent &Call) const;
- void CheckOpenVariant(CheckerContext &C,
- const CallExpr *CE, OpenVariant Variant) const;
+ void CheckOpenVariant(CheckerContext &C, const CallEvent &Call,
+ OpenVariant Variant) const;
void ReportOpenBug(CheckerContext &C, ProgramStateRef State, const char *Msg,
SourceRange SR) const;
@@ -113,9 +113,9 @@ void UnixAPIMisuseChecker::checkASTDecl(const TranslationUnitDecl *TU,
// "open" (man 2 open)
//===----------------------------------------------------------------------===/
-void UnixAPIMisuseChecker::checkPreStmt(const CallExpr *CE,
+void UnixAPIMisuseChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
- const FunctionDecl *FD = C.getCalleeDecl(CE);
+ const FunctionDecl *FD = dyn_cast_if_present<FunctionDecl>(Call.getDecl());
if (!FD || FD->getKind() != Decl::Function)
return;
@@ -130,13 +130,13 @@ void UnixAPIMisuseChecker::checkPreStmt(const CallExpr *CE,
return;
if (FName == "open")
- CheckOpen(C, CE);
+ CheckOpen(C, Call);
else if (FName == "openat")
- CheckOpenAt(C, CE);
+ CheckOpenAt(C, Call);
else if (FName == "pthread_once")
- CheckPthreadOnce(C, CE);
+ CheckPthreadOnce(C, Call);
}
void UnixAPIMisuseChecker::ReportOpenBug(CheckerContext &C,
ProgramStateRef State,
@@ -152,17 +152,17 @@ void UnixAPIMisuseChecker::ReportOpenBug(CheckerContext &C,
}
void UnixAPIMisuseChecker::CheckOpen(CheckerContext &C,
- const CallExpr *CE) const {
- CheckOpenVariant(C, CE, OpenVariant::Open);
+ const CallEvent &Call) const {
+ CheckOpenVariant(C, Call, OpenVariant::Open);
}
void UnixAPIMisuseChecker::CheckOpenAt(CheckerContext &C,
- const CallExpr *CE) const {
- CheckOpenVariant(C, CE, OpenVariant::OpenAt);
+ const CallEvent &Call) const {
+ CheckOpenVariant(C, Call, OpenVariant::OpenAt);
}
void UnixAPIMisuseChecker::CheckOpenVariant(CheckerContext &C,
- const CallExpr *CE,
+ const CallEvent &Call,
OpenVariant Variant) const {
// The index of the argument taking the flags open flags (O_RDONLY,
// O_WRONLY, O_CREAT, etc.),
@@ -191,11 +191,11 @@ void UnixAPIMisuseChecker::CheckOpenVariant(CheckerContext &C,
ProgramStateRef state = C.getState();
- if (CE->getNumArgs() < MinArgCount) {
+ if (Call.getNumArgs() < MinArgCount) {
// The frontend should issue a warning for this case. Just return.
return;
- } else if (CE->getNumArgs() == MaxArgCount) {
- const Expr *Arg = CE->getArg(CreateModeArgIndex);
+ } else if (Call.getNumArgs() == MaxArgCount) {
+ const Expr *Arg = Call.getArgExpr(CreateModeArgIndex);
QualType QT = Arg->getType();
if (!QT->isIntegerType()) {
SmallString<256> SBuf;
@@ -209,7 +209,7 @@ void UnixAPIMisuseChecker::CheckOpenVariant(CheckerContext &C,
Arg->getSourceRange());
return;
}
- } else if (CE->getNumArgs() > MaxArgCount) {
+ } else if (Call.getNumArgs() > MaxArgCount) {
SmallString<256> SBuf;
llvm::raw_svector_ostream OS(SBuf);
OS << "Call to '" << VariantName << "' with more than " << MaxArgCount
@@ -217,7 +217,7 @@ void UnixAPIMisuseChecker::CheckOpenVariant(CheckerContext &C,
ReportOpenBug(C, state,
SBuf.c_str(),
- CE->getArg(MaxArgCount)->getSourceRange());
+ Call.getArgExpr(MaxArgCount)->getSourceRange());
return;
}
@@ -226,8 +226,8 @@ void UnixAPIMisuseChecker::CheckOpenVariant(CheckerContext &C,
}
// Now check if oflags has O_CREAT set.
- const Expr *oflagsEx = CE->getArg(FlagsArgIndex);
- const SVal V = C.getSVal(oflagsEx);
+ const Expr *oflagsEx = Call.getArgExpr(FlagsArgIndex);
+ const SVal V = Call.getArgSVal(FlagsArgIndex);
if (!isa<NonLoc>(V)) {
// The case where 'V' can be a location can only be due to a bad header,
// so in this case bail out.
@@ -253,7 +253,7 @@ void UnixAPIMisuseChecker::CheckOpenVariant(CheckerContext &C,
if (!(trueState && !falseState))
return;
- if (CE->getNumArgs() < MaxArgCount) {
+ if (Call.getNumArgs() < MaxArgCount) {
SmallString<256> SBuf;
llvm::raw_svector_ostream OS(SBuf);
OS << "Call to '" << VariantName << "' requires a "
@@ -271,18 +271,18 @@ void UnixAPIMisuseChecker::CheckOpenVariant(CheckerContext &C,
//===----------------------------------------------------------------------===//
void UnixAPIMisuseChecker::CheckPthreadOnce(CheckerContext &C,
- const CallExpr *CE) const {
+ const CallEvent &Call) const {
// This is similar to 'CheckDispatchOnce' in the MacOSXAPIChecker.
// They can possibly be refactored.
- if (CE->getNumArgs() < 1)
+ if (Call.getNumArgs() < 1)
return;
// Check if the first argument is stack allocated. If so, issue a warning
// because that's likely to be bad news.
ProgramStateRef state = C.getState();
- const MemRegion *R = C.getSVal(CE->getArg(0)).getAsRegion();
+ const MemRegion *R = Call.getArgSVal(0).getAsRegion();
if (!R || !isa<StackSpaceRegion>(R->getMemorySpace()))
return;
@@ -304,7 +304,7 @@ void UnixAPIMisuseChecker::CheckPthreadOnce(CheckerContext &C,
auto report =
std::make_unique<PathSensitiveBugReport>(BT_pthreadOnce, os.str(), N);
- report->addRange(CE->getArg(0)->getSourceRange());
+ report->addRange(Call.getArgExpr(0)->getSourceRange());
C.emitReport(std::move(report));
}
>From 19b4ef7c6b4aefe67f6574ecde332cc9b338d876 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 14:46:01 +0100
Subject: [PATCH 02/18] [clang][analyzer] Model getline/getdelim preconditions
1. lineptr, n and stream can not be NULL
2. if *lineptr is NULL, *n must be 0
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 155 ++++++++-
clang/test/Analysis/getline-stream.c | 327 ++++++++++++++++++
2 files changed, 480 insertions(+), 2 deletions(-)
create mode 100644 clang/test/Analysis/getline-stream.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 10972158f39862..4bd77d3bf80346 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -21,6 +21,7 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/Sequence.h"
#include <functional>
#include <optional>
@@ -234,6 +235,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
BugType BT_StreamEof{this, "Stream already in EOF", "Stream handling error"};
BugType BT_ResourceLeak{this, "Resource leak", "Stream handling error",
/*SuppressOnSink =*/true};
+ BugType BT_SizeNull{this, "NULL size pointer", "Stream handling error"};
+ BugType BT_SizeNotZero{this, "NULL line pointer and size not zero",
+ "Stream handling error"};
public:
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
@@ -346,10 +350,10 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
{&StreamChecker::preWrite,
std::bind(&StreamChecker::evalUngetc, _1, _2, _3, _4), 1}},
{{{"getdelim"}, 4},
- {&StreamChecker::preRead,
+ {std::bind(&StreamChecker::preGetdelim, _1, _2, _3, _4),
std::bind(&StreamChecker::evalGetdelim, _1, _2, _3, _4), 3}},
{{{"getline"}, 3},
- {&StreamChecker::preRead,
+ {std::bind(&StreamChecker::preGetdelim, _1, _2, _3, _4),
std::bind(&StreamChecker::evalGetdelim, _1, _2, _3, _4), 2}},
{{{"fseek"}, 3},
{&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}},
@@ -445,6 +449,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
void evalUngetc(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const;
+ void preGetdelim(const FnDescription *Desc, const CallEvent &Call,
+ CheckerContext &C) const;
+
void evalGetdelim(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const;
@@ -519,6 +526,14 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
ProgramStateRef ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
ProgramStateRef State) const;
+ ProgramStateRef ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr,
+ CheckerContext &C, ProgramStateRef State,
+ const StringRef PtrDescr) const;
+ ProgramStateRef
+ ensureSizeZeroIfLineNull(SVal LinePtrPtrSVal, SVal SizePtrSVal,
+ const Expr *LinePtrPtrExpr, const Expr *SizePtrExpr,
+ CheckerContext &C, ProgramStateRef State) const;
+
/// Generate warning about stream in EOF state.
/// There will be always a state transition into the passed State,
/// by the new non-fatal error node or (if failed) a normal transition,
@@ -1179,6 +1194,123 @@ void StreamChecker::evalUngetc(const FnDescription *Desc, const CallEvent &Call,
C.addTransition(StateFailed);
}
+ProgramStateRef
+StreamChecker::ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr,
+ CheckerContext &C, ProgramStateRef State,
+ const StringRef PtrDescr) const {
+ const auto Ptr = PtrVal.getAs<DefinedSVal>();
+ if (!Ptr)
+ return nullptr;
+
+ assert(PtrExpr && "Expected an argument");
+
+ const auto [PtrNotNull, PtrNull] = State->assume(*Ptr);
+ if (!PtrNotNull && PtrNull) {
+ if (ExplodedNode *N = C.generateErrorNode(PtrNull)) {
+ SmallString<256> buf;
+ llvm::raw_svector_ostream os(buf);
+ os << PtrDescr << " pointer might be NULL.";
+
+ auto R = std::make_unique<PathSensitiveBugReport>(BT_SizeNull, buf, N);
+ bugreporter::trackExpressionValue(N, PtrExpr, *R);
+ C.emitReport(std::move(R));
+ }
+ return nullptr;
+ }
+
+ return PtrNotNull;
+}
+
+ProgramStateRef StreamChecker::ensureSizeZeroIfLineNull(
+ SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr,
+ const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const {
+ static constexpr char SizeNotZeroMsg[] =
+ "Line pointer might be null while n value is not zero";
+
+ // We have a pointer to a pointer to the buffer, and a pointer to the size.
+ // We want what they point at.
+ auto LinePtrSVal = getPointeeDefVal(LinePtrPtrSVal, State);
+ auto NSVal = getPointeeDefVal(SizePtrSVal, State);
+ if (!LinePtrSVal || !NSVal)
+ return nullptr;
+
+ assert(LinePtrPtrExpr &&
+ "Expected an argument with a pointer to a pointer to the buffer.");
+ assert(SizePtrExpr &&
+ "Expected an argument with a pointer to the buffer size.");
+
+ // If the line pointer is null, and n is > 0, there is UB.
+ const auto [LinePtrNotNull, LinePtrNull] = State->assume(*LinePtrSVal);
+ if (LinePtrNull && !LinePtrNotNull) {
+ const auto [NIsNotZero, NIsZero] = LinePtrNull->assume(*NSVal);
+ if (NIsNotZero && !NIsZero) {
+ if (ExplodedNode *N = C.generateErrorNode(NIsNotZero)) {
+ auto R = std::make_unique<PathSensitiveBugReport>(BT_SizeNotZero,
+ SizeNotZeroMsg, N);
+ bugreporter::trackExpressionValue(N, SizePtrExpr, *R);
+ bugreporter::trackExpressionValue(N, LinePtrPtrExpr, *R);
+ C.emitReport(std::move(R));
+ }
+ return nullptr;
+ }
+ return NIsZero;
+ }
+ return LinePtrNotNull;
+}
+
+void StreamChecker::preGetdelim(const FnDescription *Desc,
+ const CallEvent &Call,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ SVal StreamVal = getStreamArg(Desc, Call);
+
+ auto AddTransitionOnReturn = llvm::make_scope_exit([&] {
+ if (State != nullptr) {
+ C.addTransition(State);
+ }
+ });
+
+ State = ensureStreamNonNull(StreamVal, Call.getArgExpr(Desc->StreamArgNo), C,
+ State);
+ if (!State)
+ return;
+ State = ensureStreamOpened(StreamVal, C, State);
+ if (!State)
+ return;
+ State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
+ if (!State)
+ return;
+
+ // n must not be NULL
+ SVal SizePtrSval = Call.getArgSVal(1);
+ State = ensurePtrNotNull(SizePtrSval, Call.getArgExpr(1), C, State, "Size");
+ if (!State)
+ return;
+
+ // lineptr must not be NULL
+ SVal LinePtrPtrSVal = Call.getArgSVal(0);
+ State =
+ ensurePtrNotNull(LinePtrPtrSVal, Call.getArgExpr(0), C, State, "Line");
+ if (!State)
+ return;
+
+ // If lineptr points to a NULL pointer, *n must be 0
+ State =
+ ensureSizeZeroIfLineNull(LinePtrPtrSVal, SizePtrSval, Call.getArgExpr(0),
+ Call.getArgExpr(1), C, State);
+ if (!State)
+ return;
+
+ SymbolRef Sym = StreamVal.getAsSymbol();
+ if (Sym && State->get<StreamMap>(Sym)) {
+ const StreamState *SS = State->get<StreamMap>(Sym);
+ if (SS->ErrorState & ErrorFEof)
+ reportFEofWarning(Sym, C, State);
+ } else {
+ C.addTransition(State);
+ }
+}
+
void StreamChecker::evalGetdelim(const FnDescription *Desc,
const CallEvent &Call,
CheckerContext &C) const {
@@ -1204,6 +1336,20 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
State->BindExpr(E.CE, C.getLocationContext(), RetVal);
StateNotFailed =
E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call));
+ // The buffer size *n must be enough to hold the whole line, and
+ // greater than the return value, since it has to account for \0
+ auto SizePtrSval = Call.getArgSVal(1);
+ auto NVal = getPointeeDefVal(SizePtrSval, State);
+ if (NVal) {
+ StateNotFailed = StateNotFailed->assume(
+ E.SVB
+ .evalBinOp(StateNotFailed, BinaryOperator::Opcode::BO_GT, *NVal,
+ RetVal, E.SVB.getConditionType())
+ .castAs<DefinedOrUnknownSVal>(),
+ true);
+ StateNotFailed =
+ StateNotFailed->BindExpr(E.CE, C.getLocationContext(), RetVal);
+ }
if (!StateNotFailed)
return;
C.addTransition(StateNotFailed);
@@ -1217,6 +1363,11 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
E.isStreamEof() ? ErrorFEof : ErrorFEof | ErrorFError;
StateFailed = E.setStreamState(
StateFailed, StreamState::getOpened(Desc, NewES, !NewES.isFEof()));
+ // On failure, the content of the buffer is undefined
+ if (auto NewLinePtr = getPointeeDefVal(Call.getArgSVal(0), State)) {
+ StateFailed = StateFailed->bindLoc(*NewLinePtr, UndefinedVal(),
+ C.getLocationContext());
+ }
C.addTransition(StateFailed, E.getFailureNoteTag(this, C));
}
diff --git a/clang/test/Analysis/getline-stream.c b/clang/test/Analysis/getline-stream.c
new file mode 100644
index 00000000000000..b4f389e4505f73
--- /dev/null
+++ b/clang/test/Analysis/getline-stream.c
@@ -0,0 +1,327 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/system-header-simulator-for-malloc.h"
+#include "Inputs/system-header-simulator-for-valist.h"
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump_int(int);
+extern void clang_analyzer_dump_ptr(void*);
+extern void clang_analyzer_warnIfReached();
+
+void test_getline_null_file() {
+ char *buffer = NULL;
+ size_t n = 0;
+ getline(&buffer, &n, NULL); // expected-warning {{Stream pointer might be NULL}}
+}
+
+void test_getline_null_lineptr() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+
+ char **buffer = NULL;
+ size_t n = 0;
+ getline(buffer, &n, F1); // expected-warning {{Line pointer might be NULL}}
+ fclose(F1);
+}
+
+void test_getline_null_size() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getline(&buffer, NULL, F1); // expected-warning {{Size pointer might be NULL}}
+ fclose(F1);
+}
+
+void test_getline_null_buffer_bad_size() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ size_t n = 8;
+ getline(&buffer, &n, F1); // expected-warning {{Line pointer might be null while n value is not zero}}
+ fclose(F1);
+}
+
+void test_getline_null_buffer_bad_size_2(size_t n) {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ if (n > 0) {
+ getline(&buffer, &n, F1); // expected-warning {{Line pointer might be null while n value is not zero}}
+ }
+ fclose(F1);
+}
+
+void test_getline_null_buffer_unknown_size(size_t n) {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+
+ getline(&buffer, &n, F1); // ok
+ fclose(F1);
+ free(buffer);
+}
+
+void test_getline_null_buffer() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ size_t n = 0;
+ ssize_t r = getline(&buffer, &n, F1);
+ // getline returns -1 on failure, number of char reads on success (>= 0)
+ if (r < -1) {
+ clang_analyzer_warnIfReached(); // must not happen
+ } else {
+ // The buffer could be allocated both on failure and success
+ clang_analyzer_dump_int(n); // expected-warning {{conj_$}}
+ clang_analyzer_dump_ptr(buffer); // expected-warning {{conj_$}}
+ }
+ free(buffer);
+ fclose(F1);
+}
+
+void test_getdelim_null_file() {
+ char *buffer = NULL;
+ size_t n = 0;
+ getdelim(&buffer, &n, '\n', NULL); // expected-warning {{Stream pointer might be NULL}}
+}
+
+void test_getdelim_null_size() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getdelim(&buffer, NULL, ',', F1); // expected-warning {{Size pointer might be NULL}}
+ fclose(F1);
+}
+
+void test_getdelim_null_buffer_bad_size() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ size_t n = 8;
+ getdelim(&buffer, &n, ';', F1); // expected-warning {{Line pointer might be null while n value is not zero}}
+ fclose(F1);
+}
+
+void test_getdelim_null_buffer_bad_size_2(size_t n) {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ if (n > 0) {
+ getdelim(&buffer, &n, ' ', F1); // expected-warning {{Line pointer might be null while n value is not zero}}
+ }
+ fclose(F1);
+}
+
+void test_getdelim_null_buffer_unknown_size(size_t n) {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getdelim(&buffer, &n, '-', F1); // ok
+ fclose(F1);
+ free(buffer);
+}
+
+void test_getdelim_null_buffer() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ size_t n = 0;
+ ssize_t r = getdelim(&buffer, &n, '\r', F1);
+ // getdelim returns -1 on failure, number of char reads on success (>= 0)
+ if (r < -1) {
+ clang_analyzer_warnIfReached(); // must not happen
+ }
+ else {
+ // The buffer could be allocated both on failure and success
+ clang_analyzer_dump_int(n); // expected-warning {{conj_$}}
+ clang_analyzer_dump_ptr(buffer); // expected-warning {{conj_$}}
+ }
+ free(buffer);
+ fclose(F1);
+}
+
+void test_getline_while() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ char *line = NULL;
+ size_t len = 0;
+ ssize_t read;
+
+ while ((read = getline(&line, &len, file)) != -1) {
+ printf("%s\n", line);
+ }
+
+ free(line);
+ fclose(file);
+}
+
+void test_getline_no_return_check() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ char *line = NULL;
+ size_t len = 0;
+ getline(&line, &len, file);
+
+ if (line[0] == '\0') {} // expected-warning {{The left operand of '==' is a garbage value}}
+
+ free(line);
+ fclose(file);
+}
+
+void test_getline_return_check() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ char *line = NULL;
+ size_t len = 0;
+ ssize_t r = getline(&line, &len, file);
+
+ if (r != -1) {
+ if (line[0] == '\0') {} // ok
+ }
+ free(line);
+ fclose(file);
+}
+
+void test_getline_feof_check() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ char *line = NULL;
+ size_t len = 0;
+ ssize_t r = getline(&line, &len, file);
+
+ if (r != -1) {
+ // success, end-of-file is not possible
+ int f = feof(file);
+ clang_analyzer_eval(f == 0); // expected-warning {{TRUE}}
+ } else {
+ // failure, end-of-file is possible, but not the only reason to fail
+ int f = feof(file);
+ clang_analyzer_eval(f == 0); // expected-warning {{TRUE}} \\
+ expected-warning {{FALSE}}
+ }
+ free(line);
+ fclose(file);
+}
+
+void test_getline_after_eof() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ size_t n = 10;
+ char *buffer = malloc(n);
+ ssize_t read = fread(buffer, n, 1, file);
+ if (!feof(file)) {
+ getline(&buffer, &n, file); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+ }
+ fclose(file);
+ free(buffer);
+}
+
+void test_getline_feof() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ size_t n = 10;
+ char *buffer = malloc(n);
+ ssize_t read = fread(buffer, n, 1, file);
+ getline(&buffer, &n, file); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}} \\
+ expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
+ fclose(file);
+ free(buffer);
+}
+
+void test_getline_clear_eof() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ size_t n = 10;
+ char *buffer = malloc(n);
+ ssize_t read = fread(buffer, n, 1, file);
+ if (feof(file)) {
+ clearerr(file);
+ getline(&buffer, &n, file); // ok
+ }
+ fclose(file);
+ free(buffer);
+}
+
+void test_getline_not_null(char **buffer, size_t *size) {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ getline(buffer, size, file);
+ fclose(file);
+
+ if (size == NULL || buffer == NULL) {
+ clang_analyzer_warnIfReached(); // must not happen
+ }
+}
+
+void test_getline_size_0(size_t size) {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ size_t old_size = size;
+ char *buffer = NULL;
+ ssize_t r = getline(&buffer, &size, file);
+ if (r >= 0) {
+ // Since buffer is NULL, old_size should be 0. Otherwise, there would be UB.
+ clang_analyzer_eval(old_size == 0); // expected-warning{{TRUE}}
+ }
+ fclose(file);
+ free(buffer);
+}
+
+void test_getline_ret_value() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ size_t n = 0;
+ char *buffer = NULL;
+ ssize_t r = getline(&buffer, &n, file);
+
+ if (r > -1) {
+ // The return value does *not* include the terminating null byte.
+ // The buffer must be large enough to include it.
+ clang_analyzer_eval(n > r); // expected-warning{{TRUE}}
+ }
+
+ fclose(file);
+ free(buffer);
+}
>From 03c9f16856c5aa1de7a997014e0a5af92e31a7eb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Thu, 7 Mar 2024 09:55:00 +0100
Subject: [PATCH 03/18] Feedback from PR
---
.../Core/PathSensitive/CheckerHelpers.h | 1 -
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 12 ++++++------
clang/test/Analysis/getline-stream.c | 4 ++--
3 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
index 60421e5437d82f..759caaf61b891c 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
@@ -15,7 +15,6 @@
#include "ProgramState_Fwd.h"
#include "SVals.h"
-
#include "clang/AST/OperationKinds.h"
#include "clang/AST/Stmt.h"
#include "clang/Basic/OperatorKinds.h"
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 4bd77d3bf80346..c807ac21e1caf6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1281,20 +1281,20 @@ void StreamChecker::preGetdelim(const FnDescription *Desc,
if (!State)
return;
- // n must not be NULL
+ // The parameter `n` must not be NULL.
SVal SizePtrSval = Call.getArgSVal(1);
State = ensurePtrNotNull(SizePtrSval, Call.getArgExpr(1), C, State, "Size");
if (!State)
return;
- // lineptr must not be NULL
+ // The parameter `lineptr` must not be NULL.
SVal LinePtrPtrSVal = Call.getArgSVal(0);
State =
ensurePtrNotNull(LinePtrPtrSVal, Call.getArgExpr(0), C, State, "Line");
if (!State)
return;
- // If lineptr points to a NULL pointer, *n must be 0
+ // If `lineptr` points to a NULL pointer, `*n` must be 0.
State =
ensureSizeZeroIfLineNull(LinePtrPtrSVal, SizePtrSval, Call.getArgExpr(0),
Call.getArgExpr(1), C, State);
@@ -1336,8 +1336,8 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
State->BindExpr(E.CE, C.getLocationContext(), RetVal);
StateNotFailed =
E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call));
- // The buffer size *n must be enough to hold the whole line, and
- // greater than the return value, since it has to account for \0
+ // The buffer size `*n` must be enough to hold the whole line, and
+ // greater than the return value, since it has to account for '\0'.
auto SizePtrSval = Call.getArgSVal(1);
auto NVal = getPointeeDefVal(SizePtrSval, State);
if (NVal) {
@@ -1363,7 +1363,7 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
E.isStreamEof() ? ErrorFEof : ErrorFEof | ErrorFError;
StateFailed = E.setStreamState(
StateFailed, StreamState::getOpened(Desc, NewES, !NewES.isFEof()));
- // On failure, the content of the buffer is undefined
+ // On failure, the content of the buffer is undefined.
if (auto NewLinePtr = getPointeeDefVal(Call.getArgSVal(0), State)) {
StateFailed = StateFailed->bindLoc(*NewLinePtr, UndefinedVal(),
C.getLocationContext());
diff --git a/clang/test/Analysis/getline-stream.c b/clang/test/Analysis/getline-stream.c
index b4f389e4505f73..8677e35eb10d97 100644
--- a/clang/test/Analysis/getline-stream.c
+++ b/clang/test/Analysis/getline-stream.c
@@ -6,8 +6,8 @@
void clang_analyzer_eval(int);
void clang_analyzer_dump_int(int);
-extern void clang_analyzer_dump_ptr(void*);
-extern void clang_analyzer_warnIfReached();
+void clang_analyzer_dump_ptr(void*);
+void clang_analyzer_warnIfReached();
void test_getline_null_file() {
char *buffer = NULL;
>From 9d44f2ec71147621cb4e7a4228cc706ba9e0f1fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Thu, 7 Mar 2024 13:48:59 +0100
Subject: [PATCH 04/18] Update
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
Co-authored-by: Balazs Benics <benicsbalazs at gmail.com>
---
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index c807ac21e1caf6..4cd076ddc944d3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1207,11 +1207,7 @@ StreamChecker::ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr,
const auto [PtrNotNull, PtrNull] = State->assume(*Ptr);
if (!PtrNotNull && PtrNull) {
if (ExplodedNode *N = C.generateErrorNode(PtrNull)) {
- SmallString<256> buf;
- llvm::raw_svector_ostream os(buf);
- os << PtrDescr << " pointer might be NULL.";
-
- auto R = std::make_unique<PathSensitiveBugReport>(BT_SizeNull, buf, N);
+ auto R = std::make_unique<PathSensitiveBugReport>(BT_SizeNull, (PtrDescr + " pointer might be NULL.").str(), N);
bugreporter::trackExpressionValue(N, PtrExpr, *R);
C.emitReport(std::move(R));
}
>From d06ce5099bb695232c8c05a8fb05f13cd5f2b5e5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Thu, 7 Mar 2024 13:49:14 +0100
Subject: [PATCH 05/18] Update
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
Co-authored-by: Balazs Benics <benicsbalazs at gmail.com>
---
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 2 --
1 file changed, 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 4cd076ddc944d3..e3b18db5b1bcbc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1302,8 +1302,6 @@ void StreamChecker::preGetdelim(const FnDescription *Desc,
const StreamState *SS = State->get<StreamMap>(Sym);
if (SS->ErrorState & ErrorFEof)
reportFEofWarning(Sym, C, State);
- } else {
- C.addTransition(State);
}
}
>From abc092e14d59443fdfa5f487637688ebcfcf3ccf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Thu, 7 Mar 2024 13:53:25 +0100
Subject: [PATCH 06/18] Fix formatting
---
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index e3b18db5b1bcbc..9bb1c2493dbc55 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1207,7 +1207,8 @@ StreamChecker::ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr,
const auto [PtrNotNull, PtrNull] = State->assume(*Ptr);
if (!PtrNotNull && PtrNull) {
if (ExplodedNode *N = C.generateErrorNode(PtrNull)) {
- auto R = std::make_unique<PathSensitiveBugReport>(BT_SizeNull, (PtrDescr + " pointer might be NULL.").str(), N);
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ BT_SizeNull, (PtrDescr + " pointer might be NULL.").str(), N);
bugreporter::trackExpressionValue(N, PtrExpr, *R);
C.emitReport(std::move(R));
}
>From d29847dae3dd43c459c3278d8ff0303b607cfc45 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Thu, 7 Mar 2024 14:01:15 +0100
Subject: [PATCH 07/18] Add tests for a negative n parameter
---
clang/test/Analysis/getline-stream.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/clang/test/Analysis/getline-stream.c b/clang/test/Analysis/getline-stream.c
index 8677e35eb10d97..cc730c375e14e8 100644
--- a/clang/test/Analysis/getline-stream.c
+++ b/clang/test/Analysis/getline-stream.c
@@ -325,3 +325,28 @@ void test_getline_ret_value() {
fclose(file);
free(buffer);
}
+
+void test_getline_negative_buffer() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ char *buffer = NULL;
+ size_t n = -1;
+ getline(&buffer, &n, file); // expected-warning {{Line pointer might be null while n value is not zero}}
+}
+
+void test_getline_negative_buffer_2(char *buffer) {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ size_t n = -1;
+ ssize_t ret = getline(&buffer, &n, file);
+ if (ret > 0) {
+ clang_analyzer_eval(ret < n); // expected-warning {{TRUE}}
+ }
+ fclose(file);
+}
>From 808740b2840913c113118e3f52ad9421a26d77d8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Thu, 7 Mar 2024 14:03:39 +0100
Subject: [PATCH 08/18] Update
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
Co-authored-by: Balazs Benics <benicsbalazs at gmail.com>
---
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 9bb1c2493dbc55..6f745838144e07 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1338,8 +1338,7 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
if (NVal) {
StateNotFailed = StateNotFailed->assume(
E.SVB
- .evalBinOp(StateNotFailed, BinaryOperator::Opcode::BO_GT, *NVal,
- RetVal, E.SVB.getConditionType())
+ .evalBinOp(StateNotFailed, BO_GT, *NVal, RetVal, E.SVB.getConditionType())
.castAs<DefinedOrUnknownSVal>(),
true);
StateNotFailed =
>From 94ee4e52ee1087ac0e4dd72caaea7965c611a9f8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Thu, 7 Mar 2024 14:07:00 +0100
Subject: [PATCH 09/18] Fix formatting
---
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 6f745838144e07..b2fbcf3292da97 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1338,7 +1338,8 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
if (NVal) {
StateNotFailed = StateNotFailed->assume(
E.SVB
- .evalBinOp(StateNotFailed, BO_GT, *NVal, RetVal, E.SVB.getConditionType())
+ .evalBinOp(StateNotFailed, BO_GT, *NVal, RetVal,
+ E.SVB.getConditionType())
.castAs<DefinedOrUnknownSVal>(),
true);
StateNotFailed =
>From 30c56d140d44ab3f7ecac979e985ff5a1c40bdff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Mon, 11 Mar 2024 11:57:35 +0100
Subject: [PATCH 10/18] Rename ensureSizeZeroIfLineNull
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index b2fbcf3292da97..50d8a74ba0dfb2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -529,10 +529,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
ProgramStateRef ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr,
CheckerContext &C, ProgramStateRef State,
const StringRef PtrDescr) const;
- ProgramStateRef
- ensureSizeZeroIfLineNull(SVal LinePtrPtrSVal, SVal SizePtrSVal,
- const Expr *LinePtrPtrExpr, const Expr *SizePtrExpr,
- CheckerContext &C, ProgramStateRef State) const;
+ ProgramStateRef ensureGetdelimBufferAndSizeCorrect(
+ SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr,
+ const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const;
/// Generate warning about stream in EOF state.
/// There will be always a state transition into the passed State,
@@ -1218,7 +1217,7 @@ StreamChecker::ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr,
return PtrNotNull;
}
-ProgramStateRef StreamChecker::ensureSizeZeroIfLineNull(
+ProgramStateRef StreamChecker::ensureGetdelimBufferAndSizeCorrect(
SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr,
const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const {
static constexpr char SizeNotZeroMsg[] =
@@ -1291,10 +1290,9 @@ void StreamChecker::preGetdelim(const FnDescription *Desc,
if (!State)
return;
- // If `lineptr` points to a NULL pointer, `*n` must be 0.
- State =
- ensureSizeZeroIfLineNull(LinePtrPtrSVal, SizePtrSval, Call.getArgExpr(0),
- Call.getArgExpr(1), C, State);
+ State = ensureGetdelimBufferAndSizeCorrect(LinePtrPtrSVal, SizePtrSval,
+ Call.getArgExpr(0),
+ Call.getArgExpr(1), C, State);
if (!State)
return;
>From 4881b8931c2faabf71c5c3b1ff8412a8af3f5f52 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Mon, 11 Mar 2024 11:59:35 +0100
Subject: [PATCH 11/18] Simplify assert
---
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 50d8a74ba0dfb2..e4b807e4e7e8a7 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1230,10 +1230,7 @@ ProgramStateRef StreamChecker::ensureGetdelimBufferAndSizeCorrect(
if (!LinePtrSVal || !NSVal)
return nullptr;
- assert(LinePtrPtrExpr &&
- "Expected an argument with a pointer to a pointer to the buffer.");
- assert(SizePtrExpr &&
- "Expected an argument with a pointer to the buffer size.");
+ assert(LinePtrPtrExpr && SizePtrExpr);
// If the line pointer is null, and n is > 0, there is UB.
const auto [LinePtrNotNull, LinePtrNull] = State->assume(*LinePtrSVal);
>From b1b24873bafa6bc7ae48cebb8dc7213ba9be639e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Mon, 11 Mar 2024 12:02:26 +0100
Subject: [PATCH 12/18] Remove use of llvm::make_scope_exit
---
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index e4b807e4e7e8a7..4ae6265887d498 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -21,7 +21,6 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
-#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/Sequence.h"
#include <functional>
#include <optional>
@@ -1257,12 +1256,6 @@ void StreamChecker::preGetdelim(const FnDescription *Desc,
ProgramStateRef State = C.getState();
SVal StreamVal = getStreamArg(Desc, Call);
- auto AddTransitionOnReturn = llvm::make_scope_exit([&] {
- if (State != nullptr) {
- C.addTransition(State);
- }
- });
-
State = ensureStreamNonNull(StreamVal, Call.getArgExpr(Desc->StreamArgNo), C,
State);
if (!State)
@@ -1299,6 +1292,8 @@ void StreamChecker::preGetdelim(const FnDescription *Desc,
if (SS->ErrorState & ErrorFEof)
reportFEofWarning(Sym, C, State);
}
+
+ C.addTransition(State);
}
void StreamChecker::evalGetdelim(const FnDescription *Desc,
>From fd2b4bf18d5e704ebc81d004e25ece9937618f88 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Mon, 11 Mar 2024 14:16:10 +0100
Subject: [PATCH 13/18] Model getline/getdelim according to posix 2018
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 49 ++++++----
clang/test/Analysis/getline-stream.c | 94 +++++++++++++++----
2 files changed, 109 insertions(+), 34 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 4ae6265887d498..999b34d3d50a4b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -18,6 +18,7 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
@@ -235,8 +236,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
BugType BT_ResourceLeak{this, "Resource leak", "Stream handling error",
/*SuppressOnSink =*/true};
BugType BT_SizeNull{this, "NULL size pointer", "Stream handling error"};
- BugType BT_SizeNotZero{this, "NULL line pointer and size not zero",
- "Stream handling error"};
+ BugType BT_SizeGreaterThanBufferSize{
+ this, "Size greater than the allocated buffer size",
+ categories::MemoryError};
public:
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
@@ -1219,8 +1221,11 @@ StreamChecker::ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr,
ProgramStateRef StreamChecker::ensureGetdelimBufferAndSizeCorrect(
SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr,
const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const {
- static constexpr char SizeNotZeroMsg[] =
- "Line pointer might be null while n value is not zero";
+ // If the argument `*n` is non-zero, `*lineptr` must point to an object of
+ // size at least `*n` bytes, or a `NULL` pointer.
+ static constexpr char SizeGreaterThanBufferSize[] =
+ "The buffer from the first argument is smaller than the size "
+ "specified by the second parameter";
// We have a pointer to a pointer to the buffer, and a pointer to the size.
// We want what they point at.
@@ -1231,23 +1236,31 @@ ProgramStateRef StreamChecker::ensureGetdelimBufferAndSizeCorrect(
assert(LinePtrPtrExpr && SizePtrExpr);
- // If the line pointer is null, and n is > 0, there is UB.
const auto [LinePtrNotNull, LinePtrNull] = State->assume(*LinePtrSVal);
- if (LinePtrNull && !LinePtrNotNull) {
- const auto [NIsNotZero, NIsZero] = LinePtrNull->assume(*NSVal);
- if (NIsNotZero && !NIsZero) {
- if (ExplodedNode *N = C.generateErrorNode(NIsNotZero)) {
- auto R = std::make_unique<PathSensitiveBugReport>(BT_SizeNotZero,
- SizeNotZeroMsg, N);
- bugreporter::trackExpressionValue(N, SizePtrExpr, *R);
- bugreporter::trackExpressionValue(N, LinePtrPtrExpr, *R);
- C.emitReport(std::move(R));
- }
- return nullptr;
+ if (LinePtrNotNull && !LinePtrNull) {
+ auto &SVB = C.getSValBuilder();
+ auto LineBufSize =
+ getDynamicExtent(LinePtrNotNull, LinePtrSVal->getAsRegion(), SVB);
+ auto LineBufSizeGtN = SVB.evalBinOp(LinePtrNotNull, BO_GE, LineBufSize,
+ *NSVal, SVB.getConditionType())
+ .getAs<DefinedOrUnknownSVal>();
+ if (!LineBufSizeGtN) {
+ return LinePtrNotNull;
+ }
+ if (auto LineBufSizeOk = LinePtrNotNull->assume(*LineBufSizeGtN, true)) {
+ return LineBufSizeOk;
}
- return NIsZero;
+
+ if (ExplodedNode *N = C.generateErrorNode(LinePtrNotNull)) {
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ BT_SizeGreaterThanBufferSize, SizeGreaterThanBufferSize, N);
+ bugreporter::trackExpressionValue(N, SizePtrExpr, *R);
+ bugreporter::trackExpressionValue(N, LinePtrPtrExpr, *R);
+ C.emitReport(std::move(R));
+ }
+ return nullptr;
}
- return LinePtrNotNull;
+ return State;
}
void StreamChecker::preGetdelim(const FnDescription *Desc,
diff --git a/clang/test/Analysis/getline-stream.c b/clang/test/Analysis/getline-stream.c
index cc730c375e14e8..fb09f6006b4e70 100644
--- a/clang/test/Analysis/getline-stream.c
+++ b/clang/test/Analysis/getline-stream.c
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,alpha.unix.Stream,debug.ExprInspection -verify %s
#include "Inputs/system-header-simulator.h"
#include "Inputs/system-header-simulator-for-malloc.h"
@@ -35,24 +35,26 @@ void test_getline_null_size() {
fclose(F1);
}
-void test_getline_null_buffer_bad_size() {
+void test_getline_null_buffer_size_gt0() {
FILE *F1 = tmpfile();
if (!F1)
return;
char *buffer = NULL;
size_t n = 8;
- getline(&buffer, &n, F1); // expected-warning {{Line pointer might be null while n value is not zero}}
+ getline(&buffer, &n, F1); // ok since posix 2018
+ free(buffer);
fclose(F1);
}
-void test_getline_null_buffer_bad_size_2(size_t n) {
+void test_getline_null_buffer_size_gt0_2(size_t n) {
FILE *F1 = tmpfile();
if (!F1)
return;
char *buffer = NULL;
if (n > 0) {
- getline(&buffer, &n, F1); // expected-warning {{Line pointer might be null while n value is not zero}}
+ getline(&buffer, &n, F1); // ok since posix 2018
}
+ free(buffer);
fclose(F1);
}
@@ -67,6 +69,58 @@ void test_getline_null_buffer_unknown_size(size_t n) {
free(buffer);
}
+void test_getline_null_buffer_undef_size() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+
+ char *buffer = NULL;
+ size_t n;
+
+ getline(&buffer, &n, F1); // ok since posix 2018
+ fclose(F1);
+ free(buffer);
+}
+
+void test_getline_buffer_size_0() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+
+ char *buffer = malloc(10);
+ size_t n = 0;
+ if (buffer != NULL)
+ getline(&buffer, &n, F1); // ok, the buffer is enough for 0 character
+ fclose(F1);
+ free(buffer);
+}
+
+void test_getline_buffer_bad_size() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+
+ char *buffer = malloc(10);
+ size_t n = 100;
+ if (buffer != NULL)
+ getline(&buffer, &n, F1); // expected-warning {{The buffer from the first argument is smaller than the size specified by the second parameter}}
+ fclose(F1);
+ free(buffer);
+}
+
+void test_getline_buffer_smaller_size() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+
+ char *buffer = malloc(100);
+ size_t n = 10;
+ if (buffer != NULL)
+ getline(&buffer, &n, F1); // ok, there is enough space for 10 characters
+ fclose(F1);
+ free(buffer);
+}
+
void test_getline_null_buffer() {
FILE *F1 = tmpfile();
if (!F1)
@@ -101,24 +155,26 @@ void test_getdelim_null_size() {
fclose(F1);
}
-void test_getdelim_null_buffer_bad_size() {
+void test_getdelim_null_buffer_size_gt0() {
FILE *F1 = tmpfile();
if (!F1)
return;
char *buffer = NULL;
size_t n = 8;
- getdelim(&buffer, &n, ';', F1); // expected-warning {{Line pointer might be null while n value is not zero}}
+ getdelim(&buffer, &n, ';', F1); // ok since posix 2018
+ free(buffer);
fclose(F1);
}
-void test_getdelim_null_buffer_bad_size_2(size_t n) {
+void test_getdelim_null_buffer_size_gt0_2(size_t n) {
FILE *F1 = tmpfile();
if (!F1)
return;
char *buffer = NULL;
if (n > 0) {
- getdelim(&buffer, &n, ' ', F1); // expected-warning {{Line pointer might be null while n value is not zero}}
+ getdelim(&buffer, &n, ' ', F1); // ok since posix 2018
}
+ free(buffer);
fclose(F1);
}
@@ -289,18 +345,21 @@ void test_getline_not_null(char **buffer, size_t *size) {
}
}
-void test_getline_size_0(size_t size) {
+void test_getline_size_constraint(size_t size) {
FILE *file = fopen("file.txt", "r");
if (file == NULL) {
return;
}
size_t old_size = size;
- char *buffer = NULL;
- ssize_t r = getline(&buffer, &size, file);
- if (r >= 0) {
- // Since buffer is NULL, old_size should be 0. Otherwise, there would be UB.
- clang_analyzer_eval(old_size == 0); // expected-warning{{TRUE}}
+ char *buffer = malloc(10);
+ if (buffer != NULL) {
+ ssize_t r = getline(&buffer, &size, file);
+ if (r >= 0) {
+ // Since buffer has a size of 10, old_size must be less than or equal to 10.
+ // Otherwise, there would be UB.
+ clang_analyzer_eval(old_size <= 10); // expected-warning{{TRUE}}
+ }
}
fclose(file);
free(buffer);
@@ -334,7 +393,9 @@ void test_getline_negative_buffer() {
char *buffer = NULL;
size_t n = -1;
- getline(&buffer, &n, file); // expected-warning {{Line pointer might be null while n value is not zero}}
+ getline(&buffer, &n, file); // ok since posix 2018
+ free(buffer);
+ fclose(file);
}
void test_getline_negative_buffer_2(char *buffer) {
@@ -348,5 +409,6 @@ void test_getline_negative_buffer_2(char *buffer) {
if (ret > 0) {
clang_analyzer_eval(ret < n); // expected-warning {{TRUE}}
}
+ free(buffer);
fclose(file);
}
>From 31907d6a34c2da71303b7fb7ce8e527b4b747f96 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Mon, 11 Mar 2024 15:16:59 +0100
Subject: [PATCH 14/18] Report on undefined size with non-null buffer
---
.../Core/PathSensitive/CheckerHelpers.h | 3 +-
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 8 +--
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 50 ++++++++++++-------
.../StaticAnalyzer/Core/CheckerHelpers.cpp | 5 +-
clang/test/Analysis/getline-stream.c | 14 ++++++
5 files changed, 54 insertions(+), 26 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
index 759caaf61b891c..d053a97189123a 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
@@ -112,8 +112,7 @@ class OperatorKind {
OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
bool IsBinary);
-std::optional<DefinedSVal> getPointeeDefVal(SVal PtrSVal,
- ProgramStateRef State);
+std::optional<SVal> getPointeeVal(SVal PtrSVal, ProgramStateRef State);
} // namespace ento
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 03cb7696707fe2..c2d96f59260906 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1441,7 +1441,7 @@ void MallocChecker::preGetdelim(const CallEvent &Call,
return;
ProgramStateRef State = C.getState();
- const auto LinePtr = getPointeeDefVal(Call.getArgSVal(0), State);
+ const auto LinePtr = getPointeeVal(Call.getArgSVal(0), State);
if (!LinePtr)
return;
@@ -1470,8 +1470,10 @@ void MallocChecker::checkGetdelim(const CallEvent &Call,
SValBuilder &SVB = C.getSValBuilder();
- const auto LinePtr = getPointeeDefVal(Call.getArgSVal(0), State);
- const auto Size = getPointeeDefVal(Call.getArgSVal(1), State);
+ const auto LinePtr =
+ getPointeeVal(Call.getArgSVal(0), State)->getAs<DefinedSVal>();
+ const auto Size =
+ getPointeeVal(Call.getArgSVal(1), State)->getAs<DefinedSVal>();
if (!LinePtr || !Size || !LinePtr->getAsRegion())
return;
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 999b34d3d50a4b..6248dccdaf1796 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -236,9 +236,7 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
BugType BT_ResourceLeak{this, "Resource leak", "Stream handling error",
/*SuppressOnSink =*/true};
BugType BT_SizeNull{this, "NULL size pointer", "Stream handling error"};
- BugType BT_SizeGreaterThanBufferSize{
- this, "Size greater than the allocated buffer size",
- categories::MemoryError};
+ BugType BT_IllegalSize{this, "Invalid buffer size", categories::MemoryError};
public:
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
@@ -1221,28 +1219,50 @@ StreamChecker::ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr,
ProgramStateRef StreamChecker::ensureGetdelimBufferAndSizeCorrect(
SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr,
const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const {
- // If the argument `*n` is non-zero, `*lineptr` must point to an object of
- // size at least `*n` bytes, or a `NULL` pointer.
static constexpr char SizeGreaterThanBufferSize[] =
"The buffer from the first argument is smaller than the size "
"specified by the second parameter";
+ static constexpr char SizeUndef[] =
+ "The buffer from the first argument is not NULL, but the size specified "
+ "by the second parameter is undefined.";
+
+ auto EmitBugReport = [this, &C, SizePtrExpr,
+ LinePtrPtrExpr](const ProgramStateRef &BugState,
+ const char *ErrMsg) {
+ if (ExplodedNode *N = C.generateErrorNode(BugState)) {
+ auto R =
+ std::make_unique<PathSensitiveBugReport>(BT_IllegalSize, ErrMsg, N);
+ bugreporter::trackExpressionValue(N, SizePtrExpr, *R);
+ bugreporter::trackExpressionValue(N, LinePtrPtrExpr, *R);
+ C.emitReport(std::move(R));
+ }
+ };
// We have a pointer to a pointer to the buffer, and a pointer to the size.
// We want what they point at.
- auto LinePtrSVal = getPointeeDefVal(LinePtrPtrSVal, State);
- auto NSVal = getPointeeDefVal(SizePtrSVal, State);
- if (!LinePtrSVal || !NSVal)
+ auto LinePtrSVal = getPointeeVal(LinePtrPtrSVal, State)->getAs<DefinedSVal>();
+ auto NSVal = getPointeeVal(SizePtrSVal, State);
+ if (!LinePtrSVal || !NSVal || NSVal->isUnknown())
return nullptr;
assert(LinePtrPtrExpr && SizePtrExpr);
const auto [LinePtrNotNull, LinePtrNull] = State->assume(*LinePtrSVal);
if (LinePtrNotNull && !LinePtrNull) {
+ // If `*lineptr` is not null, but `*n` is undefined, there is UB.
+ if (NSVal->isUndef()) {
+ EmitBugReport(LinePtrNotNull, SizeUndef);
+ return nullptr;
+ }
+
+ // If it is defined, and known, its size must be less than or equal to
+ // the buffer size.
+ auto NDefSVal = NSVal->getAs<DefinedSVal>();
auto &SVB = C.getSValBuilder();
auto LineBufSize =
getDynamicExtent(LinePtrNotNull, LinePtrSVal->getAsRegion(), SVB);
auto LineBufSizeGtN = SVB.evalBinOp(LinePtrNotNull, BO_GE, LineBufSize,
- *NSVal, SVB.getConditionType())
+ *NDefSVal, SVB.getConditionType())
.getAs<DefinedOrUnknownSVal>();
if (!LineBufSizeGtN) {
return LinePtrNotNull;
@@ -1251,13 +1271,7 @@ ProgramStateRef StreamChecker::ensureGetdelimBufferAndSizeCorrect(
return LineBufSizeOk;
}
- if (ExplodedNode *N = C.generateErrorNode(LinePtrNotNull)) {
- auto R = std::make_unique<PathSensitiveBugReport>(
- BT_SizeGreaterThanBufferSize, SizeGreaterThanBufferSize, N);
- bugreporter::trackExpressionValue(N, SizePtrExpr, *R);
- bugreporter::trackExpressionValue(N, LinePtrPtrExpr, *R);
- C.emitReport(std::move(R));
- }
+ EmitBugReport(LinePtrNotNull, SizeGreaterThanBufferSize);
return nullptr;
}
return State;
@@ -1337,7 +1351,7 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
// The buffer size `*n` must be enough to hold the whole line, and
// greater than the return value, since it has to account for '\0'.
auto SizePtrSval = Call.getArgSVal(1);
- auto NVal = getPointeeDefVal(SizePtrSval, State);
+ auto NVal = getPointeeVal(SizePtrSval, State);
if (NVal) {
StateNotFailed = StateNotFailed->assume(
E.SVB
@@ -1362,7 +1376,7 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
StateFailed = E.setStreamState(
StateFailed, StreamState::getOpened(Desc, NewES, !NewES.isFEof()));
// On failure, the content of the buffer is undefined.
- if (auto NewLinePtr = getPointeeDefVal(Call.getArgSVal(0), State)) {
+ if (auto NewLinePtr = getPointeeVal(Call.getArgSVal(0), State)) {
StateFailed = StateFailed->bindLoc(*NewLinePtr, UndefinedVal(),
C.getLocationContext());
}
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp b/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
index 364c87e910b7b5..d7137a915b3d3d 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -183,10 +183,9 @@ OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
}
}
-std::optional<DefinedSVal> getPointeeDefVal(SVal PtrSVal,
- ProgramStateRef State) {
+std::optional<SVal> getPointeeVal(SVal PtrSVal, ProgramStateRef State) {
if (const auto *Ptr = PtrSVal.getAsRegion()) {
- return State->getSVal(Ptr).getAs<DefinedSVal>();
+ return State->getSVal(Ptr);
}
return std::nullopt;
}
diff --git a/clang/test/Analysis/getline-stream.c b/clang/test/Analysis/getline-stream.c
index fb09f6006b4e70..996e8228847ab1 100644
--- a/clang/test/Analysis/getline-stream.c
+++ b/clang/test/Analysis/getline-stream.c
@@ -121,6 +121,20 @@ void test_getline_buffer_smaller_size() {
free(buffer);
}
+void test_getline_buffer_undef_size() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+
+ char *buffer = malloc(100);
+ size_t n;
+ if (buffer != NULL)
+ getline(&buffer, &n, F1); // expected-warning {{The buffer from the first argument is not NULL, but the size specified by the second parameter is undefined}}
+ fclose(F1);
+ free(buffer);
+}
+
+
void test_getline_null_buffer() {
FILE *F1 = tmpfile();
if (!F1)
>From 36e36baca26a22c4767e9d88f47426a69de83a21 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
<alejandro.alvarez at sonarsource.com>
Date: Mon, 11 Mar 2024 15:54:57 +0100
Subject: [PATCH 15/18] Refactor so ensureStreamNonNull calls ensurePtrNotNull
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 65 +++++++------------
1 file changed, 24 insertions(+), 41 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 6248dccdaf1796..a8330dff5506eb 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -235,7 +235,7 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
BugType BT_StreamEof{this, "Stream already in EOF", "Stream handling error"};
BugType BT_ResourceLeak{this, "Resource leak", "Stream handling error",
/*SuppressOnSink =*/true};
- BugType BT_SizeNull{this, "NULL size pointer", "Stream handling error"};
+ BugType BT_ArgumentNull{this, "NULL pointer", categories::UnixAPI};
BugType BT_IllegalSize{this, "Invalid buffer size", categories::MemoryError};
public:
@@ -502,6 +502,12 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
CheckerContext &C,
ProgramStateRef State) const;
+ ProgramStateRef
+ ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr, CheckerContext &C,
+ ProgramStateRef State, const StringRef PtrDescr,
+ std::optional<std::reference_wrapper<const BugType>> BT =
+ std::nullopt) const;
+
/// Check that the stream is the opened state.
/// If the stream is known to be not opened an error is generated
/// and nullptr returned, otherwise the original state is returned.
@@ -525,9 +531,6 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
ProgramStateRef ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
ProgramStateRef State) const;
- ProgramStateRef ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr,
- CheckerContext &C, ProgramStateRef State,
- const StringRef PtrDescr) const;
ProgramStateRef ensureGetdelimBufferAndSizeCorrect(
SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr,
const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const;
@@ -1192,30 +1195,6 @@ void StreamChecker::evalUngetc(const FnDescription *Desc, const CallEvent &Call,
C.addTransition(StateFailed);
}
-ProgramStateRef
-StreamChecker::ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr,
- CheckerContext &C, ProgramStateRef State,
- const StringRef PtrDescr) const {
- const auto Ptr = PtrVal.getAs<DefinedSVal>();
- if (!Ptr)
- return nullptr;
-
- assert(PtrExpr && "Expected an argument");
-
- const auto [PtrNotNull, PtrNull] = State->assume(*Ptr);
- if (!PtrNotNull && PtrNull) {
- if (ExplodedNode *N = C.generateErrorNode(PtrNull)) {
- auto R = std::make_unique<PathSensitiveBugReport>(
- BT_SizeNull, (PtrDescr + " pointer might be NULL.").str(), N);
- bugreporter::trackExpressionValue(N, PtrExpr, *R);
- C.emitReport(std::move(R));
- }
- return nullptr;
- }
-
- return PtrNotNull;
-}
-
ProgramStateRef StreamChecker::ensureGetdelimBufferAndSizeCorrect(
SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr,
const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const {
@@ -1697,27 +1676,31 @@ ProgramStateRef
StreamChecker::ensureStreamNonNull(SVal StreamVal, const Expr *StreamE,
CheckerContext &C,
ProgramStateRef State) const {
- auto Stream = StreamVal.getAs<DefinedSVal>();
- if (!Stream)
- return State;
-
- ConstraintManager &CM = C.getConstraintManager();
+ return ensurePtrNotNull(StreamVal, StreamE, C, State, "Stream", BT_FileNull);
+}
- ProgramStateRef StateNotNull, StateNull;
- std::tie(StateNotNull, StateNull) = CM.assumeDual(State, *Stream);
+ProgramStateRef StreamChecker::ensurePtrNotNull(
+ SVal PtrVal, const Expr *PtrExpr, CheckerContext &C, ProgramStateRef State,
+ const StringRef PtrDescr,
+ std::optional<std::reference_wrapper<const BugType>> BT) const {
+ const auto Ptr = PtrVal.getAs<DefinedSVal>();
+ if (!Ptr)
+ return State;
- if (!StateNotNull && StateNull) {
- if (ExplodedNode *N = C.generateErrorNode(StateNull)) {
+ const auto [PtrNotNull, PtrNull] = State->assume(*Ptr);
+ if (!PtrNotNull && PtrNull) {
+ if (ExplodedNode *N = C.generateErrorNode(PtrNull)) {
auto R = std::make_unique<PathSensitiveBugReport>(
- BT_FileNull, "Stream pointer might be NULL.", N);
- if (StreamE)
- bugreporter::trackExpressionValue(N, StreamE, *R);
+ BT.value_or(std::cref(BT_ArgumentNull)),
+ (PtrDescr + " pointer might be NULL.").str(), N);
+ if (PtrExpr)
+ bugreporter::trackExpressionValue(N, PtrExpr, *R);
C.emitReport(std::move(R));
}
return nullptr;
}
- return StateNotNull;
+ return PtrNotNull;
}
ProgramStateRef StreamChecker::ensureStreamOpened(SVal StreamVal,
>From 976cf7cbe5a65145525f45784065ef278f43f767 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, 20 Mar 2024 10:57:02 +0100
Subject: [PATCH 16/18] Feedback from review
---
.../lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index a8330dff5506eb..ce0c0dc1f6dc02 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1198,16 +1198,15 @@ void StreamChecker::evalUngetc(const FnDescription *Desc, const CallEvent &Call,
ProgramStateRef StreamChecker::ensureGetdelimBufferAndSizeCorrect(
SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr,
const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const {
- static constexpr char SizeGreaterThanBufferSize[] =
+ static constexpr llvm::StringLiteral SizeGreaterThanBufferSize =
"The buffer from the first argument is smaller than the size "
"specified by the second parameter";
- static constexpr char SizeUndef[] =
+ static constexpr llvm::StringLiteral SizeUndef =
"The buffer from the first argument is not NULL, but the size specified "
"by the second parameter is undefined.";
- auto EmitBugReport = [this, &C, SizePtrExpr,
- LinePtrPtrExpr](const ProgramStateRef &BugState,
- const char *ErrMsg) {
+ auto EmitBugReport = [this, &C, SizePtrExpr, LinePtrPtrExpr](
+ ProgramStateRef BugState, StringRef ErrMsg) {
if (ExplodedNode *N = C.generateErrorNode(BugState)) {
auto R =
std::make_unique<PathSensitiveBugReport>(BT_IllegalSize, ErrMsg, N);
@@ -1243,12 +1242,10 @@ ProgramStateRef StreamChecker::ensureGetdelimBufferAndSizeCorrect(
auto LineBufSizeGtN = SVB.evalBinOp(LinePtrNotNull, BO_GE, LineBufSize,
*NDefSVal, SVB.getConditionType())
.getAs<DefinedOrUnknownSVal>();
- if (!LineBufSizeGtN) {
+ if (!LineBufSizeGtN)
return LinePtrNotNull;
- }
- if (auto LineBufSizeOk = LinePtrNotNull->assume(*LineBufSizeGtN, true)) {
+ if (auto LineBufSizeOk = LinePtrNotNull->assume(*LineBufSizeGtN, true))
return LineBufSizeOk;
- }
EmitBugReport(LinePtrNotNull, SizeGreaterThanBufferSize);
return nullptr;
>From 6b55204de73e1039237c9bb69847aed740c62fb8 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, 20 Mar 2024 11:47:13 +0100
Subject: [PATCH 17/18] Move getdelim/getline preconditions to UnixAPIChecker
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 154 ++----------------
.../Checkers/UnixAPIChecker.cpp | 130 +++++++++++++++
.../{getline-stream.c => getline-unixapi.c} | 0
3 files changed, 145 insertions(+), 139 deletions(-)
rename clang/test/Analysis/{getline-stream.c => getline-unixapi.c} (100%)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index ce0c0dc1f6dc02..31cd0598e75250 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -18,7 +18,6 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
@@ -235,8 +234,6 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
BugType BT_StreamEof{this, "Stream already in EOF", "Stream handling error"};
BugType BT_ResourceLeak{this, "Resource leak", "Stream handling error",
/*SuppressOnSink =*/true};
- BugType BT_ArgumentNull{this, "NULL pointer", categories::UnixAPI};
- BugType BT_IllegalSize{this, "Invalid buffer size", categories::MemoryError};
public:
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
@@ -349,10 +346,10 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
{&StreamChecker::preWrite,
std::bind(&StreamChecker::evalUngetc, _1, _2, _3, _4), 1}},
{{{"getdelim"}, 4},
- {std::bind(&StreamChecker::preGetdelim, _1, _2, _3, _4),
+ {&StreamChecker::preRead,
std::bind(&StreamChecker::evalGetdelim, _1, _2, _3, _4), 3}},
{{{"getline"}, 3},
- {std::bind(&StreamChecker::preGetdelim, _1, _2, _3, _4),
+ {&StreamChecker::preRead,
std::bind(&StreamChecker::evalGetdelim, _1, _2, _3, _4), 2}},
{{{"fseek"}, 3},
{&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}},
@@ -448,9 +445,6 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
void evalUngetc(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const;
- void preGetdelim(const FnDescription *Desc, const CallEvent &Call,
- CheckerContext &C) const;
-
void evalGetdelim(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const;
@@ -502,12 +496,6 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
CheckerContext &C,
ProgramStateRef State) const;
- ProgramStateRef
- ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr, CheckerContext &C,
- ProgramStateRef State, const StringRef PtrDescr,
- std::optional<std::reference_wrapper<const BugType>> BT =
- std::nullopt) const;
-
/// Check that the stream is the opened state.
/// If the stream is known to be not opened an error is generated
/// and nullptr returned, otherwise the original state is returned.
@@ -531,10 +519,6 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
ProgramStateRef ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
ProgramStateRef State) const;
- ProgramStateRef ensureGetdelimBufferAndSizeCorrect(
- SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr,
- const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const;
-
/// Generate warning about stream in EOF state.
/// There will be always a state transition into the passed State,
/// by the new non-fatal error node or (if failed) a normal transition,
@@ -1195,110 +1179,6 @@ void StreamChecker::evalUngetc(const FnDescription *Desc, const CallEvent &Call,
C.addTransition(StateFailed);
}
-ProgramStateRef StreamChecker::ensureGetdelimBufferAndSizeCorrect(
- SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr,
- const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const {
- static constexpr llvm::StringLiteral SizeGreaterThanBufferSize =
- "The buffer from the first argument is smaller than the size "
- "specified by the second parameter";
- static constexpr llvm::StringLiteral SizeUndef =
- "The buffer from the first argument is not NULL, but the size specified "
- "by the second parameter is undefined.";
-
- auto EmitBugReport = [this, &C, SizePtrExpr, LinePtrPtrExpr](
- ProgramStateRef BugState, StringRef ErrMsg) {
- if (ExplodedNode *N = C.generateErrorNode(BugState)) {
- auto R =
- std::make_unique<PathSensitiveBugReport>(BT_IllegalSize, ErrMsg, N);
- bugreporter::trackExpressionValue(N, SizePtrExpr, *R);
- bugreporter::trackExpressionValue(N, LinePtrPtrExpr, *R);
- C.emitReport(std::move(R));
- }
- };
-
- // We have a pointer to a pointer to the buffer, and a pointer to the size.
- // We want what they point at.
- auto LinePtrSVal = getPointeeVal(LinePtrPtrSVal, State)->getAs<DefinedSVal>();
- auto NSVal = getPointeeVal(SizePtrSVal, State);
- if (!LinePtrSVal || !NSVal || NSVal->isUnknown())
- return nullptr;
-
- assert(LinePtrPtrExpr && SizePtrExpr);
-
- const auto [LinePtrNotNull, LinePtrNull] = State->assume(*LinePtrSVal);
- if (LinePtrNotNull && !LinePtrNull) {
- // If `*lineptr` is not null, but `*n` is undefined, there is UB.
- if (NSVal->isUndef()) {
- EmitBugReport(LinePtrNotNull, SizeUndef);
- return nullptr;
- }
-
- // If it is defined, and known, its size must be less than or equal to
- // the buffer size.
- auto NDefSVal = NSVal->getAs<DefinedSVal>();
- auto &SVB = C.getSValBuilder();
- auto LineBufSize =
- getDynamicExtent(LinePtrNotNull, LinePtrSVal->getAsRegion(), SVB);
- auto LineBufSizeGtN = SVB.evalBinOp(LinePtrNotNull, BO_GE, LineBufSize,
- *NDefSVal, SVB.getConditionType())
- .getAs<DefinedOrUnknownSVal>();
- if (!LineBufSizeGtN)
- return LinePtrNotNull;
- if (auto LineBufSizeOk = LinePtrNotNull->assume(*LineBufSizeGtN, true))
- return LineBufSizeOk;
-
- EmitBugReport(LinePtrNotNull, SizeGreaterThanBufferSize);
- return nullptr;
- }
- return State;
-}
-
-void StreamChecker::preGetdelim(const FnDescription *Desc,
- const CallEvent &Call,
- CheckerContext &C) const {
- ProgramStateRef State = C.getState();
- SVal StreamVal = getStreamArg(Desc, Call);
-
- State = ensureStreamNonNull(StreamVal, Call.getArgExpr(Desc->StreamArgNo), C,
- State);
- if (!State)
- return;
- State = ensureStreamOpened(StreamVal, C, State);
- if (!State)
- return;
- State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
- if (!State)
- return;
-
- // The parameter `n` must not be NULL.
- SVal SizePtrSval = Call.getArgSVal(1);
- State = ensurePtrNotNull(SizePtrSval, Call.getArgExpr(1), C, State, "Size");
- if (!State)
- return;
-
- // The parameter `lineptr` must not be NULL.
- SVal LinePtrPtrSVal = Call.getArgSVal(0);
- State =
- ensurePtrNotNull(LinePtrPtrSVal, Call.getArgExpr(0), C, State, "Line");
- if (!State)
- return;
-
- State = ensureGetdelimBufferAndSizeCorrect(LinePtrPtrSVal, SizePtrSval,
- Call.getArgExpr(0),
- Call.getArgExpr(1), C, State);
- if (!State)
- return;
-
- SymbolRef Sym = StreamVal.getAsSymbol();
- if (Sym && State->get<StreamMap>(Sym)) {
- const StreamState *SS = State->get<StreamMap>(Sym);
- if (SS->ErrorState & ErrorFEof)
- reportFEofWarning(Sym, C, State);
- }
-
- C.addTransition(State);
-}
-
void StreamChecker::evalGetdelim(const FnDescription *Desc,
const CallEvent &Call,
CheckerContext &C) const {
@@ -1673,31 +1553,27 @@ ProgramStateRef
StreamChecker::ensureStreamNonNull(SVal StreamVal, const Expr *StreamE,
CheckerContext &C,
ProgramStateRef State) const {
- return ensurePtrNotNull(StreamVal, StreamE, C, State, "Stream", BT_FileNull);
-}
-
-ProgramStateRef StreamChecker::ensurePtrNotNull(
- SVal PtrVal, const Expr *PtrExpr, CheckerContext &C, ProgramStateRef State,
- const StringRef PtrDescr,
- std::optional<std::reference_wrapper<const BugType>> BT) const {
- const auto Ptr = PtrVal.getAs<DefinedSVal>();
- if (!Ptr)
+ auto Stream = StreamVal.getAs<DefinedSVal>();
+ if (!Stream)
return State;
- const auto [PtrNotNull, PtrNull] = State->assume(*Ptr);
- if (!PtrNotNull && PtrNull) {
- if (ExplodedNode *N = C.generateErrorNode(PtrNull)) {
+ ConstraintManager &CM = C.getConstraintManager();
+
+ ProgramStateRef StateNotNull, StateNull;
+ std::tie(StateNotNull, StateNull) = CM.assumeDual(State, *Stream);
+
+ if (!StateNotNull && StateNull) {
+ if (ExplodedNode *N = C.generateErrorNode(StateNull)) {
auto R = std::make_unique<PathSensitiveBugReport>(
- BT.value_or(std::cref(BT_ArgumentNull)),
- (PtrDescr + " pointer might be NULL.").str(), N);
- if (PtrExpr)
- bugreporter::trackExpressionValue(N, PtrExpr, *R);
+ BT_FileNull, "Stream pointer might be NULL.", N);
+ if (StreamE)
+ bugreporter::trackExpressionValue(N, StreamE, *R);
C.emitReport(std::move(R));
}
return nullptr;
}
- return PtrNotNull;
+ return StateNotNull;
}
ProgramStateRef StreamChecker::ensureStreamOpened(SVal StreamVal,
diff --git a/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
index 599e5e6cedc606..026826e7b15d87 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
@@ -20,6 +20,7 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringExtras.h"
@@ -44,10 +45,23 @@ namespace {
class UnixAPIMisuseChecker
: public Checker<check::PreCall, check::ASTDecl<TranslationUnitDecl>> {
const BugType BT_open{this, "Improper use of 'open'", categories::UnixAPI};
+ const BugType BT_getline{this, "Improper use of getdelim",
+ categories::UnixAPI};
const BugType BT_pthreadOnce{this, "Improper use of 'pthread_once'",
categories::UnixAPI};
+ const BugType BT_ArgumentNull{this, "NULL pointer", categories::UnixAPI};
mutable std::optional<uint64_t> Val_O_CREAT;
+ ProgramStateRef
+ EnsurePtrNotNull(SVal PtrVal, const Expr *PtrExpr, CheckerContext &C,
+ ProgramStateRef State, const StringRef PtrDescr,
+ std::optional<std::reference_wrapper<const BugType>> BT =
+ std::nullopt) const;
+
+ ProgramStateRef EnsureGetdelimBufferAndSizeCorrect(
+ SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr,
+ const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const;
+
public:
void checkASTDecl(const TranslationUnitDecl *TU, AnalysisManager &Mgr,
BugReporter &BR) const;
@@ -56,6 +70,7 @@ class UnixAPIMisuseChecker
void CheckOpen(CheckerContext &C, const CallEvent &Call) const;
void CheckOpenAt(CheckerContext &C, const CallEvent &Call) const;
+ void CheckGetDelim(CheckerContext &C, const CallEvent &Call) const;
void CheckPthreadOnce(CheckerContext &C, const CallEvent &Call) const;
void CheckOpenVariant(CheckerContext &C, const CallEvent &Call,
@@ -95,6 +110,30 @@ class UnixAPIPortabilityChecker : public Checker< check::PreStmt<CallExpr> > {
} // end anonymous namespace
+ProgramStateRef UnixAPIMisuseChecker::EnsurePtrNotNull(
+ SVal PtrVal, const Expr *PtrExpr, CheckerContext &C, ProgramStateRef State,
+ const StringRef PtrDescr,
+ std::optional<std::reference_wrapper<const BugType>> BT) const {
+ const auto Ptr = PtrVal.getAs<DefinedSVal>();
+ if (!Ptr)
+ return State;
+
+ const auto [PtrNotNull, PtrNull] = State->assume(*Ptr);
+ if (!PtrNotNull && PtrNull) {
+ if (ExplodedNode *N = C.generateErrorNode(PtrNull)) {
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ BT.value_or(std::cref(BT_ArgumentNull)),
+ (PtrDescr + " pointer might be NULL.").str(), N);
+ if (PtrExpr)
+ bugreporter::trackExpressionValue(N, PtrExpr, *R);
+ C.emitReport(std::move(R));
+ }
+ return nullptr;
+ }
+
+ return PtrNotNull;
+}
+
void UnixAPIMisuseChecker::checkASTDecl(const TranslationUnitDecl *TU,
AnalysisManager &Mgr,
BugReporter &) const {
@@ -137,6 +176,9 @@ void UnixAPIMisuseChecker::checkPreCall(const CallEvent &Call,
else if (FName == "pthread_once")
CheckPthreadOnce(C, Call);
+
+ else if (is_contained({"getdelim", "getline"}, FName))
+ CheckGetDelim(C, Call);
}
void UnixAPIMisuseChecker::ReportOpenBug(CheckerContext &C,
ProgramStateRef State,
@@ -266,6 +308,94 @@ void UnixAPIMisuseChecker::CheckOpenVariant(CheckerContext &C,
}
}
+//===----------------------------------------------------------------------===//
+// getdelim and getline
+//===----------------------------------------------------------------------===//
+
+ProgramStateRef UnixAPIMisuseChecker::EnsureGetdelimBufferAndSizeCorrect(
+ SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr,
+ const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const {
+ static constexpr llvm::StringLiteral SizeGreaterThanBufferSize =
+ "The buffer from the first argument is smaller than the size "
+ "specified by the second parameter";
+ static constexpr llvm::StringLiteral SizeUndef =
+ "The buffer from the first argument is not NULL, but the size specified "
+ "by the second parameter is undefined.";
+
+ auto EmitBugReport = [this, &C, SizePtrExpr, LinePtrPtrExpr](
+ ProgramStateRef BugState, StringRef ErrMsg) {
+ if (ExplodedNode *N = C.generateErrorNode(BugState)) {
+ auto R = std::make_unique<PathSensitiveBugReport>(BT_getline, ErrMsg, N);
+ bugreporter::trackExpressionValue(N, SizePtrExpr, *R);
+ bugreporter::trackExpressionValue(N, LinePtrPtrExpr, *R);
+ C.emitReport(std::move(R));
+ }
+ };
+
+ // We have a pointer to a pointer to the buffer, and a pointer to the size.
+ // We want what they point at.
+ auto LinePtrSVal = getPointeeVal(LinePtrPtrSVal, State)->getAs<DefinedSVal>();
+ auto NSVal = getPointeeVal(SizePtrSVal, State);
+ if (!LinePtrSVal || !NSVal || NSVal->isUnknown())
+ return nullptr;
+
+ assert(LinePtrPtrExpr && SizePtrExpr);
+
+ const auto [LinePtrNotNull, LinePtrNull] = State->assume(*LinePtrSVal);
+ if (LinePtrNotNull && !LinePtrNull) {
+ // If `*lineptr` is not null, but `*n` is undefined, there is UB.
+ if (NSVal->isUndef()) {
+ EmitBugReport(LinePtrNotNull, SizeUndef);
+ return nullptr;
+ }
+
+ // If it is defined, and known, its size must be less than or equal to
+ // the buffer size.
+ auto NDefSVal = NSVal->getAs<DefinedSVal>();
+ auto &SVB = C.getSValBuilder();
+ auto LineBufSize =
+ getDynamicExtent(LinePtrNotNull, LinePtrSVal->getAsRegion(), SVB);
+ auto LineBufSizeGtN = SVB.evalBinOp(LinePtrNotNull, BO_GE, LineBufSize,
+ *NDefSVal, SVB.getConditionType())
+ .getAs<DefinedOrUnknownSVal>();
+ if (!LineBufSizeGtN)
+ return LinePtrNotNull;
+ if (auto LineBufSizeOk = LinePtrNotNull->assume(*LineBufSizeGtN, true))
+ return LineBufSizeOk;
+
+ EmitBugReport(LinePtrNotNull, SizeGreaterThanBufferSize);
+ return nullptr;
+ }
+ return State;
+}
+
+void UnixAPIMisuseChecker::CheckGetDelim(CheckerContext &C,
+ const CallEvent &Call) const {
+ ProgramStateRef State = C.getState();
+
+ // The parameter `n` must not be NULL.
+ SVal SizePtrSval = Call.getArgSVal(1);
+ State = EnsurePtrNotNull(SizePtrSval, Call.getArgExpr(1), C, State, "Size");
+ if (!State)
+ return;
+
+ // The parameter `lineptr` must not be NULL.
+ SVal LinePtrPtrSVal = Call.getArgSVal(0);
+ State =
+ EnsurePtrNotNull(LinePtrPtrSVal, Call.getArgExpr(0), C, State, "Line");
+ if (!State)
+ return;
+
+ State = EnsureGetdelimBufferAndSizeCorrect(LinePtrPtrSVal, SizePtrSval,
+ Call.getArgExpr(0),
+ Call.getArgExpr(1), C, State);
+ if (!State)
+ return;
+
+ C.addTransition(State);
+}
+
+
//===----------------------------------------------------------------------===//
// pthread_once
//===----------------------------------------------------------------------===//
diff --git a/clang/test/Analysis/getline-stream.c b/clang/test/Analysis/getline-unixapi.c
similarity index 100%
rename from clang/test/Analysis/getline-stream.c
rename to clang/test/Analysis/getline-unixapi.c
>From 5c3cfdeb86006553dc25ecb7e8e3d413099ce14b 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, 20 Mar 2024 11:59:38 +0100
Subject: [PATCH 18/18] Reorganize the tests
---
clang/test/Analysis/getline-unixapi.c | 110 +----------------------
clang/test/Analysis/stream.c | 120 ++++++++++++++++++++++++++
2 files changed, 122 insertions(+), 108 deletions(-)
diff --git a/clang/test/Analysis/getline-unixapi.c b/clang/test/Analysis/getline-unixapi.c
index 996e8228847ab1..86635ed8499793 100644
--- a/clang/test/Analysis/getline-unixapi.c
+++ b/clang/test/Analysis/getline-unixapi.c
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,alpha.unix.Stream,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection -verify %s
#include "Inputs/system-header-simulator.h"
#include "Inputs/system-header-simulator-for-malloc.h"
@@ -9,12 +9,6 @@ void clang_analyzer_dump_int(int);
void clang_analyzer_dump_ptr(void*);
void clang_analyzer_warnIfReached();
-void test_getline_null_file() {
- char *buffer = NULL;
- size_t n = 0;
- getline(&buffer, &n, NULL); // expected-warning {{Stream pointer might be NULL}}
-}
-
void test_getline_null_lineptr() {
FILE *F1 = tmpfile();
if (!F1)
@@ -154,12 +148,6 @@ void test_getline_null_buffer() {
fclose(F1);
}
-void test_getdelim_null_file() {
- char *buffer = NULL;
- size_t n = 0;
- getdelim(&buffer, &n, '\n', NULL); // expected-warning {{Stream pointer might be NULL}}
-}
-
void test_getdelim_null_size() {
FILE *F1 = tmpfile();
if (!F1)
@@ -240,22 +228,6 @@ void test_getline_while() {
fclose(file);
}
-void test_getline_no_return_check() {
- FILE *file = fopen("file.txt", "r");
- if (file == NULL) {
- return;
- }
-
- char *line = NULL;
- size_t len = 0;
- getline(&line, &len, file);
-
- if (line[0] == '\0') {} // expected-warning {{The left operand of '==' is a garbage value}}
-
- free(line);
- fclose(file);
-}
-
void test_getline_return_check() {
FILE *file = fopen("file.txt", "r");
if (file == NULL) {
@@ -273,61 +245,6 @@ void test_getline_return_check() {
fclose(file);
}
-void test_getline_feof_check() {
- FILE *file = fopen("file.txt", "r");
- if (file == NULL) {
- return;
- }
-
- char *line = NULL;
- size_t len = 0;
- ssize_t r = getline(&line, &len, file);
-
- if (r != -1) {
- // success, end-of-file is not possible
- int f = feof(file);
- clang_analyzer_eval(f == 0); // expected-warning {{TRUE}}
- } else {
- // failure, end-of-file is possible, but not the only reason to fail
- int f = feof(file);
- clang_analyzer_eval(f == 0); // expected-warning {{TRUE}} \\
- expected-warning {{FALSE}}
- }
- free(line);
- fclose(file);
-}
-
-void test_getline_after_eof() {
- FILE *file = fopen("file.txt", "r");
- if (file == NULL) {
- return;
- }
-
- size_t n = 10;
- char *buffer = malloc(n);
- ssize_t read = fread(buffer, n, 1, file);
- if (!feof(file)) {
- getline(&buffer, &n, file); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
- }
- fclose(file);
- free(buffer);
-}
-
-void test_getline_feof() {
- FILE *file = fopen("file.txt", "r");
- if (file == NULL) {
- return;
- }
-
- size_t n = 10;
- char *buffer = malloc(n);
- ssize_t read = fread(buffer, n, 1, file);
- getline(&buffer, &n, file); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}} \\
- expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
- fclose(file);
- free(buffer);
-}
-
void test_getline_clear_eof() {
FILE *file = fopen("file.txt", "r");
if (file == NULL) {
@@ -379,26 +296,6 @@ void test_getline_size_constraint(size_t size) {
free(buffer);
}
-void test_getline_ret_value() {
- FILE *file = fopen("file.txt", "r");
- if (file == NULL) {
- return;
- }
-
- size_t n = 0;
- char *buffer = NULL;
- ssize_t r = getline(&buffer, &n, file);
-
- if (r > -1) {
- // The return value does *not* include the terminating null byte.
- // The buffer must be large enough to include it.
- clang_analyzer_eval(n > r); // expected-warning{{TRUE}}
- }
-
- fclose(file);
- free(buffer);
-}
-
void test_getline_negative_buffer() {
FILE *file = fopen("file.txt", "r");
if (file == NULL) {
@@ -419,10 +316,7 @@ void test_getline_negative_buffer_2(char *buffer) {
}
size_t n = -1;
- ssize_t ret = getline(&buffer, &n, file);
- if (ret > 0) {
- clang_analyzer_eval(ret < n); // expected-warning {{TRUE}}
- }
+ (void)getline(&buffer, &n, file); // ok
free(buffer);
fclose(file);
}
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index 7ba27740a93796..4a87eea72c31d4 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -4,6 +4,7 @@
// RUN: %clang_analyze_cc1 -triple=hexagon -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
#include "Inputs/system-header-simulator.h"
+#include "Inputs/system-header-simulator-for-malloc.h"
#include "Inputs/system-header-simulator-for-valist.h"
void clang_analyzer_eval(int);
@@ -376,3 +377,122 @@ void fflush_on_open_failed_stream(void) {
}
fclose(F);
}
+
+void getline_null_file() {
+ char *buffer = NULL;
+ size_t n = 0;
+ getline(&buffer, &n, NULL); // expected-warning {{Stream pointer might be NULL}}
+}
+
+void getdelim_null_file() {
+ char *buffer = NULL;
+ size_t n = 0;
+ getdelim(&buffer, &n, '\n', NULL); // expected-warning {{Stream pointer might be NULL}}
+}
+
+void getline_no_return_check() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ char *line = NULL;
+ size_t len = 0;
+ getline(&line, &len, file);
+
+ if (line[0] == '\0') {} // expected-warning {{The left operand of '==' is a garbage value}}
+
+ free(line);
+ fclose(file);
+}
+
+void getline_after_eof() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ size_t n = 10;
+ char *buffer = malloc(n);
+ ssize_t read = fread(buffer, n, 1, file);
+ if (!feof(file)) {
+ getline(&buffer, &n, file); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+ }
+ fclose(file);
+ free(buffer);
+}
+
+void getline_feof() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ size_t n = 10;
+ char *buffer = malloc(n);
+ ssize_t read = fread(buffer, n, 1, file);
+ getline(&buffer, &n, file); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}} \\
+ expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
+ fclose(file);
+ free(buffer);
+}
+
+void getline_feof_check() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ char *line = NULL;
+ size_t len = 0;
+ ssize_t r = getline(&line, &len, file);
+
+ if (r != -1) {
+ // success, end-of-file is not possible
+ int f = feof(file);
+ clang_analyzer_eval(f == 0); // expected-warning {{TRUE}}
+ } else {
+ // failure, end-of-file is possible, but not the only reason to fail
+ int f = feof(file);
+ clang_analyzer_eval(f == 0); // expected-warning {{TRUE}} \\
+ expected-warning {{FALSE}}
+ }
+ free(line);
+ fclose(file);
+}
+
+void getline_ret_value() {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ size_t n = 0;
+ char *buffer = NULL;
+ ssize_t r = getline(&buffer, &n, file);
+
+ if (r > -1) {
+ // The return value does *not* include the terminating null byte.
+ // The buffer must be large enough to include it.
+ clang_analyzer_eval(n > r); // expected-warning{{TRUE}}
+ }
+
+ fclose(file);
+ free(buffer);
+}
+
+
+void getline_buffer_size_invariant(char *buffer) {
+ FILE *file = fopen("file.txt", "r");
+ if (file == NULL) {
+ return;
+ }
+
+ size_t n = -1;
+ ssize_t ret = getline(&buffer, &n, file);
+ if (ret > 0) {
+ clang_analyzer_eval(ret < n); // expected-warning {{TRUE}}
+ }
+ free(buffer);
+ fclose(file);
+}
More information about the cfe-commits
mailing list