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.alva...@sonarsource.com> Date: Wed, 20 Mar 2024 10:49:08 +0100 Subject: [PATCH 01/23] [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.alva...@sonarsource.com> Date: Wed, 21 Feb 2024 14:46:01 +0100 Subject: [PATCH 02/23] [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.alva...@sonarsource.com> Date: Thu, 7 Mar 2024 09:55:00 +0100 Subject: [PATCH 03/23] 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.alva...@sonarsource.com> Date: Thu, 7 Mar 2024 13:48:59 +0100 Subject: [PATCH 04/23] Update clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp Co-authored-by: Balazs Benics <benicsbal...@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.alva...@sonarsource.com> Date: Thu, 7 Mar 2024 13:49:14 +0100 Subject: [PATCH 05/23] Update clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp Co-authored-by: Balazs Benics <benicsbal...@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.alva...@sonarsource.com> Date: Thu, 7 Mar 2024 13:53:25 +0100 Subject: [PATCH 06/23] 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.alva...@sonarsource.com> Date: Thu, 7 Mar 2024 14:01:15 +0100 Subject: [PATCH 07/23] 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.alva...@sonarsource.com> Date: Thu, 7 Mar 2024 14:03:39 +0100 Subject: [PATCH 08/23] Update clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp Co-authored-by: Balazs Benics <benicsbal...@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.alva...@sonarsource.com> Date: Thu, 7 Mar 2024 14:07:00 +0100 Subject: [PATCH 09/23] 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.alva...@sonarsource.com> Date: Mon, 11 Mar 2024 11:57:35 +0100 Subject: [PATCH 10/23] 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.alva...@sonarsource.com> Date: Mon, 11 Mar 2024 11:59:35 +0100 Subject: [PATCH 11/23] 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.alva...@sonarsource.com> Date: Mon, 11 Mar 2024 12:02:26 +0100 Subject: [PATCH 12/23] 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.alva...@sonarsource.com> Date: Mon, 11 Mar 2024 14:16:10 +0100 Subject: [PATCH 13/23] 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.alva...@sonarsource.com> Date: Mon, 11 Mar 2024 15:16:59 +0100 Subject: [PATCH 14/23] 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.alva...@sonarsource.com> Date: Mon, 11 Mar 2024 15:54:57 +0100 Subject: [PATCH 15/23] 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.alva...@sonarsource.com> Date: Wed, 20 Mar 2024 10:57:02 +0100 Subject: [PATCH 16/23] 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.alva...@sonarsource.com> Date: Wed, 20 Mar 2024 11:47:13 +0100 Subject: [PATCH 17/23] 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.alva...@sonarsource.com> Date: Wed, 20 Mar 2024 11:59:38 +0100 Subject: [PATCH 18/23] 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); +} From e831489a2d1d1c515fb8077aef195b7b35baa951 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Wed, 20 Mar 2024 12:19:15 +0100 Subject: [PATCH 19/23] Fix formatting --- clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp index 026826e7b15d87..da2d16ca9b5dd7 100644 --- a/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp @@ -257,8 +257,7 @@ void UnixAPIMisuseChecker::CheckOpenVariant(CheckerContext &C, OS << "Call to '" << VariantName << "' with more than " << MaxArgCount << " arguments"; - ReportOpenBug(C, state, - SBuf.c_str(), + ReportOpenBug(C, state, SBuf.c_str(), Call.getArgExpr(MaxArgCount)->getSourceRange()); return; } @@ -395,7 +394,6 @@ void UnixAPIMisuseChecker::CheckGetDelim(CheckerContext &C, C.addTransition(State); } - //===----------------------------------------------------------------------===// // pthread_once //===----------------------------------------------------------------------===// From 22308591b913f7586532e26af08300a545cdde69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Thu, 21 Mar 2024 09:37:51 +0100 Subject: [PATCH 20/23] Feedback from review --- .../StaticAnalyzer/Checkers/StreamChecker.cpp | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 31cd0598e75250..dbeecdd6151cd1 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -1206,17 +1206,12 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc, 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); + SVal SizePtrSval = Call.getArgSVal(1); auto NVal = getPointeeVal(SizePtrSval, State); - if (NVal) { - StateNotFailed = StateNotFailed->assume( - E.SVB - .evalBinOp(StateNotFailed, BO_GT, *NVal, RetVal, - E.SVB.getConditionType()) - .castAs<DefinedOrUnknownSVal>(), - true); - StateNotFailed = - StateNotFailed->BindExpr(E.CE, C.getLocationContext(), RetVal); + if (NVal && isa<NonLoc>(*NVal)) { + StateNotFailed = E.assumeBinOpNN(StateNotFailed, BO_GT, + NVal->castAs<NonLoc>(), RetVal); + StateNotFailed = E.bindReturnValue(StateNotFailed, C, RetVal); } if (!StateNotFailed) return; @@ -1232,10 +1227,9 @@ 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 = getPointeeVal(Call.getArgSVal(0), State)) { + if (auto NewLinePtr = getPointeeVal(Call.getArgSVal(0), State)) StateFailed = StateFailed->bindLoc(*NewLinePtr, UndefinedVal(), C.getLocationContext()); - } C.addTransition(StateFailed, E.getFailureNoteTag(this, C)); } From 184f3a40ee9c268759e54c0c0ffdc5183f1487f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Thu, 21 Mar 2024 16:49:30 +0100 Subject: [PATCH 21/23] Feedback from review --- clang/test/Analysis/stream.c | 64 ++++-------------------------------- 1 file changed, 6 insertions(+), 58 deletions(-) diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c index 4a87eea72c31d4..5892aa2ab24c3f 100644 --- a/clang/test/Analysis/stream.c +++ b/clang/test/Analysis/stream.c @@ -390,7 +390,7 @@ void getdelim_null_file() { getdelim(&buffer, &n, '\n', NULL); // expected-warning {{Stream pointer might be NULL}} } -void getline_no_return_check() { +void getline_buffer_on_error() { FILE *file = fopen("file.txt", "r"); if (file == NULL) { return; @@ -398,65 +398,12 @@ void getline_no_return_check() { 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}} + if (getline(&line, &len, file) == -1) { + if (line[0] == '\0') {} // expected-warning {{The left operand of '==' is a garbage value}} } 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}} + if (line[0] == '\0') {} // no warning } + free(line); fclose(file); } @@ -475,6 +422,7 @@ void getline_ret_value() { // 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}} + clang_analyzer_eval(buffer != NULL); // expected-warning{{TRUE}} } fclose(file); From 4fb71203fd84d608ee6e470ec78bff2f8f736c52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Thu, 21 Mar 2024 17:14:31 +0100 Subject: [PATCH 22/23] Then getline succeeds, the buffer must be allocated --- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index dbeecdd6151cd1..902c42a2799be4 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -1200,10 +1200,16 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc, // Add transition for the successful state. NonLoc RetVal = makeRetVal(C, E.CE).castAs<NonLoc>(); - ProgramStateRef StateNotFailed = - State->BindExpr(E.CE, C.getLocationContext(), RetVal); + ProgramStateRef StateNotFailed = E.bindReturnValue(State, C, RetVal); StateNotFailed = E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call)); + + // On success, a buffer is allocated. + auto NewLinePtr = getPointeeVal(Call.getArgSVal(0), State); + if (NewLinePtr && isa<DefinedOrUnknownSVal>(*NewLinePtr)) + StateNotFailed = StateNotFailed->assume( + NewLinePtr->castAs<DefinedOrUnknownSVal>(), true); + // 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'. SVal SizePtrSval = Call.getArgSVal(1); From 98417dc135384d99eea437be299db4dfd51a0e7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Fri, 22 Mar 2024 09:24:24 +0100 Subject: [PATCH 23/23] Rework test --- clang/test/Analysis/stream.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c index 5892aa2ab24c3f..3945d556854cbd 100644 --- a/clang/test/Analysis/stream.c +++ b/clang/test/Analysis/stream.c @@ -430,17 +430,22 @@ void getline_ret_value() { } -void getline_buffer_size_invariant(char *buffer) { +void getline_buffer_size_negative() { 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}} + clang_analyzer_eval((ssize_t)n >= 0); // expected-warning{{FALSE}} + char *buffer = NULL; + ssize_t r = getline(&buffer, &n, file); + + if (r > -1) { + clang_analyzer_eval((ssize_t)n > r); // expected-warning{{TRUE}} + clang_analyzer_eval(buffer != NULL); // expected-warning{{TRUE}} } + free(buffer); fclose(file); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits