[clang][analyzer] Model more getline/getdelim pre and postconditions #83027
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Alejandro Álvarez Ayllón (alejandro-alvarez-sonarsource) Changes
This patch models Full diff: https://github.com/llvm/llvm-project/pull/83027.diff 5 Files Affected:
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);
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
alejandro-alvarez-sonarsource
force-pushed
the
aa/getline_precond
branch
from
5e62483 to
efd1411
Compare
alejandro-alvarez-sonarsource
force-pushed
the
aa/getline_precond
branch
from
af04492 to
9975018
Compare
|
Rebased on top of main and solved conflicts. |
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
Outdated
Show resolved
Hide resolved
alejandro-alvarez-sonarsource
force-pushed
the
aa/getline_precond
branch
from
ede56f0 to
414fd9b
Compare
|
Rebased on top of main to solve conflicts. |
|
|
|
Additionally, the checked preconditions look not exact. For example the POSIX documentation for |
Ah! I was following at the 2008 version, and there "[...], or is a null pointer." is missing. I will update accordingly. I wonder, though, if the pointer is NULL, would an undefined (as non-initialized) *n be acceptable? |
There was a problem hiding this comment.
This functionality could be added to this checker, but to StdLibraryFunctionsChecker too, and probably will be added at a time (summary of getdelim is not accurate now in that checker). The same bug condition is checked by two different checkers in that case. Otherwise I prefer to add such checks (for NULL arguments) to StdLibraryFunctionsChecker.
| ProgramStateRef | ||
| StreamChecker::ensurePtrNotNull(SVal PtrVal, const Expr *PtrExpr, | ||
| CheckerContext &C, ProgramStateRef State, | ||
| const StringRef PtrDescr) const { |
There was a problem hiding this comment.
This looks like a generally usable function, for example if buffer in fread or fwrite is checked for null pointer. For this a special bug report for "NULL size" does not work, but a generic NULL pointer bug report can work. Probably this function can be merged with ensureStreamNonNull too.
There was a problem hiding this comment.
ensureStreamNonNull now calls ensurePtrNotNull, so the logic is shared.
| @@ -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", | |||
There was a problem hiding this comment.
I do not like that there is a separate bug type for all kinds of NULL pointers (NULL stream, NULL "size pointer", NULL "line pointer". One common NULL pointer bug type can be enough (probably another for NULL stream). And these new bug types are not really "stream handling errors".
There was a problem hiding this comment.
Replaced with BT_NullPtr.
| CheckerContext &C, ProgramStateRef State, | ||
| const StringRef PtrDescr) const; | ||
| ProgramStateRef | ||
| ensureSizeZeroIfLineNull(SVal LinePtrPtrSVal, SVal SizePtrSVal, |
There was a problem hiding this comment.
Name of this function is not accurate, this is a special function used only for getdelim and this is better to be included in the name (like at ensureFseekWhenceCorrect).
There was a problem hiding this comment.
Renamed to ensureGetdelimBufferAndSizeCorrect
| 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."); |
There was a problem hiding this comment.
These assert messages look too long, really it checks only if the argument is there, not that if it points to a correct value.
There was a problem hiding this comment.
I agree. We could just do the assertion without any string attached. It's clear what they express anyways.
There was a problem hiding this comment.
Simplified to a single assert.
| 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"; |
There was a problem hiding this comment.
Instead of "Line pointer" and "n" using "argument X" (X=index of argument) may be better, if the user does not see the function declaration.
There was a problem hiding this comment.
I have followed this advise after changing the function to follow POSIX 2018.
| ProgramStateRef State = C.getState(); | ||
| SVal StreamVal = getStreamArg(Desc, Call); | ||
|
|
||
| auto AddTransitionOnReturn = llvm::make_scope_exit([&] { |
There was a problem hiding this comment.
This looks not needed because the transition is added only at the end of the function, all other return statements are with a null State pointer. It is really shorter to have a single addTransition call at the end.
There was a problem hiding this comment.
Indeed. This is a leftover from unrelated downstream changes. I have removed it.
Thanks for the review! Awesome recommendations. |
|
I want to avoid that some functions have null pointer checks in |
I can really think of having these checks at both checkers, so that both parties are sort of happy - except for of course having this precondition checked at both places. But to me, the question is really as follow: is it better with this, or not? EDIT: One more thing to clarify. I don't want to be pushy. We are absolutely fine not merging this. |
With 9db5a4a now this behavior is modeled. As for where to model the preconditions. So, it seems removing them from I think the latter is a reasonable compromise so |
|
@balazske would you agree with my proposal of keeping this logic in |
I like more if the new checks are moved to Probably it would be better if |
|
Just chiming in to cast a supporting vote for the idea that
@balazske Are you interested in refactoring the logic of |
| 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."; |
There was a problem hiding this comment.
For constexpr strings consider using the type llvm::StringLiteral which is a subclass of StringRef and computes the length in compile time.
(And don't confuse it with clang::StringLiteral which is an AST node 😉 .)
There was a problem hiding this comment.
Fixed.
| "by the second parameter is undefined."; | ||
|
|
||
| auto EmitBugReport = [this, &C, SizePtrExpr, | ||
| LinePtrPtrExpr](const ProgramStateRef &BugState, |
There was a problem hiding this comment.
| LinePtrPtrExpr](const ProgramStateRef &BugState, | |
| LinePtrPtrExpr](ProgramStateRef BugState, |
ProgramStateRef is almost always passed by value in existing code, so consider using that style. (By the way, ProgramStateRef a typedef for IntrusiveRefCntPtr<const ProgramState>; so I think it's premature optimization to pass it by const reference.)
There was a problem hiding this comment.
Changed.
| if (!LineBufSizeGtN) { | ||
| return LinePtrNotNull; | ||
| } | ||
| if (auto LineBufSizeOk = LinePtrNotNull->assume(*LineBufSizeGtN, true)) { | ||
| return LineBufSizeOk; | ||
| } |
There was a problem hiding this comment.
| if (!LineBufSizeGtN) { | |
| return LinePtrNotNull; | |
| } | |
| if (auto LineBufSizeOk = LinePtrNotNull->assume(*LineBufSizeGtN, true)) { | |
| return LineBufSizeOk; | |
| } | |
| if (!LineBufSizeGtN) | |
| return LinePtrNotNull; | |
| if (auto LineBufSizeOk = LinePtrNotNull->assume(*LineBufSizeGtN, true)) | |
| return LineBufSizeOk; |
Unbrace simple conditionals (https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements).
I could try it. It would solve at least the (dependency) difficulties related to this checker. Probably the checker can remain and contain the functions that are evaluated with |
alejandro-alvarez-sonarsource
force-pushed
the
aa/getline_precond
branch
from
6d440b7 to
5c3cfde
Compare
|
Sorry for the force-push, but I have moved now the precondition checks to The idea would be to rebase, keep the |
| ProgramStateRef StreamChecker::ensureGetdelimBufferAndSizeCorrect( | ||
| SVal LinePtrPtrSVal, SVal SizePtrSVal, const Expr *LinePtrPtrExpr, | ||
| const Expr *SizePtrExpr, CheckerContext &C, ProgramStateRef State) const { | ||
| static constexpr char SizeGreaterThanBufferSize[] = |
There was a problem hiding this comment.
The static keyword is not needed here?
There was a problem hiding this comment.
Now it uses llvm:StringLiteral, but I think it (relatively) is. Most existing uses of constexpr llvm::StringLiteral in llvm are with static. And, IIUC, without static, the variable is going to live on the stack and be created each time (even though its value was already computed at compile time).
Probably a moot point, though, since this is not part of a hotpath. For consistency with other uses, I'd rather keep it, though.
| @@ -1204,6 +1204,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); | |||
There was a problem hiding this comment.
Probably this should not have type auto (it is used only if the real type is visible or the type name is very long).
There was a problem hiding this comment.
Changed.
| .castAs<DefinedOrUnknownSVal>(), | ||
| true); | ||
| StateNotFailed = | ||
| StateNotFailed->BindExpr(E.CE, C.getLocationContext(), RetVal); |
There was a problem hiding this comment.
These calls can be replaced with E.assumeBinOpNN and E.bindReturnValue.
There was a problem hiding this comment.
Replaced, thanks!
| true); | ||
| StateNotFailed = | ||
| StateNotFailed->BindExpr(E.CE, C.getLocationContext(), RetVal); | ||
| } |
There was a problem hiding this comment.
I do not see a test that checks for the relation between return value and the "size" value.
There was a problem hiding this comment.
In stream.c, the test getline_buffer_size_invariant
| if (auto NewLinePtr = getPointeeVal(Call.getArgSVal(0), State)) { | ||
| StateFailed = StateFailed->bindLoc(*NewLinePtr, UndefinedVal(), | ||
| C.getLocationContext()); | ||
| } |
There was a problem hiding this comment.
Braces are not needed a this place.
There was a problem hiding this comment.
Removed.
clang/test/Analysis/stream.c
Outdated
| size_t len = 0; | ||
| getline(&line, &len, file); | ||
|
|
||
| if (line[0] == '\0') {} // expected-warning {{The left operand of '==' is a garbage value}} |
There was a problem hiding this comment.
I like better if this check is included only in the failure case:
if (getline(&line, &len, file) == -1) {
if (line[0] == '\0') {} // expected-warning {{The left operand of '==' is a garbage value}}
} else {
if (line[0] == '\0') {} // no warning
}
There was a problem hiding this comment.
Changed.
clang/test/Analysis/stream.c
Outdated
| } | ||
|
|
||
|
|
||
| void getline_buffer_size_invariant(char *buffer) { |
There was a problem hiding this comment.
This test looks interesting: It is not really different from the previous one, except that the buffer is not an initialized value. Because n is -1 the buffer should be assumed to be NULL before the call. This is not a realistic code because n should be an indication of the buffer size but this value is not known to the function. I think this test (in this form) is not needed.
There was a problem hiding this comment.
I added it because @steakhal was concerned about the handling of this combination of parameters.
There was a problem hiding this comment.
My concern was mostly that buffer is an argument and n is not and this looks not relevant for this test case, so if a char *buffer = NULL would be added (and the argument removed) it would look better. Otherwise an assert can be added to the code after the place where the second assumption on return value is made. I think too that the invalidation of n solves this problem.
There was a problem hiding this comment.
I have reworked the test, including a couple of calls to clang_analyze_eval to make sure the -1 is changed after the call.
| 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}} |
There was a problem hiding this comment.
Should check that buffer != NULL is true.
There was a problem hiding this comment.
Added, and fixed the bug. Good catch, thanks.
clang/test/Analysis/stream.c
Outdated
| fclose(file); | ||
| } | ||
|
|
||
| void getline_after_eof() { |
There was a problem hiding this comment.
I think getline_after_eof, getline_feof, getline_feof_check are checking not much more cases than error_getline (and error_getdelim) in stream_error.c and can be removed. Even if not removed, such tests (that check feof and ferror and related warnings) should put into stream_error.c.
There was a problem hiding this comment.
Fair. I have removed them.
clang/test/Analysis/stream.c
Outdated
|
|
||
| if (r > -1) { | ||
| clang_analyzer_eval((ssize_t)n > r); // expected-warning{{TRUE}} | ||
| clang_analyzer_eval(buffer != NULL); // expected-warning{{TRUE}} |
There was a problem hiding this comment.
This looks like an indentation problem.
There was a problem hiding this comment.
Replaced with spaces.
According to POSIX 2018. 1. lineptr, n and stream can not be NULL. 2. If *n is non-zero, *lineptr must point to a region of at least *n bytes, or be a NULL pointer. Additionally, if *lineptr is not NULL, *n must not be undefined.
alejandro-alvarez-sonarsource
force-pushed
the
aa/getline_precond
branch
from
364e293 to
f692b86
Compare
|
Rebased and squashed into two commits that should be kept separate. |
…#83027) According to POSIX 2018. 1. lineptr, n and stream can not be NULL. 2. If *n is non-zero, *lineptr must point to a region of at least *n bytes, or be a NULL pointer. Additionally, if *lineptr is not NULL, *n must not be undefined.
|
Rebase merged. |
…llvm#83027) According to POSIX 2018. 1. lineptr, n and stream can not be NULL. 2. If *n is non-zero, *lineptr must point to a region of at least *n bytes, or be a NULL pointer. Additionally, if *lineptr is not NULL, *n must not be undefined.
lineptr,nandstreamcan not beNULL*lineptrisNULL,*nmust be 0This patch models
getlinespecific 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 affectsMallocChecker, for which I plan to submit a separate PR, self-contained and independent of this one.