llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Alejandro Álvarez Ayllón (alejandro-alvarez-sonarsource) <details> <summary>Changes</summary> 1. `lineptr`, `n` and `stream` can not be `NULL` 2. if `*lineptr` is `NULL`, `*n` must be 0 This patch models `getline` specific preconditions, constraints the size to be greater than the return value on success --- since the former must include the null terminator ---, and sets the buffer to uninitialized on failure --- since it may be a freshly allocated memory area. The modeling of the allocating behavior affects `MallocChecker`, for which I plan to submit a separate PR, self-contained and independent of this one. --- Full diff: https://github.com/llvm/llvm-project/pull/83027.diff 5 Files Affected: - (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h (+6) - (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+153-2) - (modified) clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp (+9) - (modified) clang/test/Analysis/Inputs/system-header-simulator-for-malloc.h (+1) - (added) clang/test/Analysis/getline-stream.c (+327) ``````````diff diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h index 65982457ad8393..60421e5437d82f 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h @@ -13,6 +13,9 @@ #ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CHECKERHELPERS_H #define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CHECKERHELPERS_H +#include "ProgramState_Fwd.h" +#include "SVals.h" + #include "clang/AST/OperationKinds.h" #include "clang/AST/Stmt.h" #include "clang/Basic/OperatorKinds.h" @@ -110,6 +113,9 @@ class OperatorKind { OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK, bool IsBinary); +std::optional<DefinedSVal> getPointeeDefVal(SVal PtrSVal, + ProgramStateRef State); + } // namespace ento } // namespace clang diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 65bdc4cac30940..b4b828784c9725 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> @@ -312,6 +313,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; @@ -364,10 +368,10 @@ class StreamChecker : public Checker<check::PreCall, eval::Call, {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false), std::bind(&StreamChecker::evalUngetc, _1, _2, _3, _4), 1}}, {{{"getdelim"}, 4}, - {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true), + {std::bind(&StreamChecker::preGetdelim, _1, _2, _3, _4), std::bind(&StreamChecker::evalGetdelim, _1, _2, _3, _4), 3}}, {{{"getline"}, 3}, - {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true), + {std::bind(&StreamChecker::preGetdelim, _1, _2, _3, _4), std::bind(&StreamChecker::evalGetdelim, _1, _2, _3, _4), 2}}, {{{"fseek"}, 3}, {&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}}, @@ -452,6 +456,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; @@ -526,6 +533,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, @@ -1091,6 +1106,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 { @@ -1116,6 +1248,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); @@ -1129,6 +1275,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()); + } if (E.isStreamEof()) C.addTransition(StateFailed, constructSetEofNoteTag(C, E.StreamSym)); else diff --git a/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp b/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp index 84ad20a5480792..364c87e910b7b5 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp @@ -14,6 +14,7 @@ #include "clang/AST/Decl.h" #include "clang/AST/Expr.h" #include "clang/Lex/Preprocessor.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" #include <optional> namespace clang { @@ -182,5 +183,13 @@ OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK, } } +std::optional<DefinedSVal> getPointeeDefVal(SVal PtrSVal, + ProgramStateRef State) { + if (const auto *Ptr = PtrSVal.getAsRegion()) { + return State->getSVal(Ptr).getAs<DefinedSVal>(); + } + return std::nullopt; +} + } // namespace ento } // namespace clang diff --git a/clang/test/Analysis/Inputs/system-header-simulator-for-malloc.h b/clang/test/Analysis/Inputs/system-header-simulator-for-malloc.h index e76455655e9e2e..bc7009eb0d1bea 100644 --- a/clang/test/Analysis/Inputs/system-header-simulator-for-malloc.h +++ b/clang/test/Analysis/Inputs/system-header-simulator-for-malloc.h @@ -9,6 +9,7 @@ typedef __typeof(sizeof(int)) size_t; void *malloc(size_t); void *calloc(size_t, size_t); void free(void *); +void *alloca(size_t); #if __OBJC__ diff --git a/clang/test/Analysis/getline-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); +} `````````` </details> https://github.com/llvm/llvm-project/pull/83027 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
