[analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite
Add the BufferSize argument constraint to fread and fwrite. This change
itself makes it possible to discover a security critical case, described
in SEI-CERT ARR38-C.
We also add the not-null constraint on the 3rd arguments.
In this patch, I also remove those lambdas that don't take any
parameters (Fwrite, Fread, Getc), thus making the code better
structured.
Differential Revision: https://reviews.llvm.org/D87081
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index a61af45..cbc048b 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -349,6 +349,9 @@
def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">,
HelpText<"Improve modeling of the C standard library functions">,
+ // Uninitialized value check is a mandatory dependency. This Checker asserts
+ // that arguments are always initialized.
+ Dependencies<[CallAndMessageModeling]>,
CheckerOptions<[
CmdLineOption<Boolean,
"DisplayLoadedSummaries",
diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index f5ad809..45711ca 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -1090,35 +1090,12 @@
Optional<QualType> FilePtrRestrictTy = getRestrictTy(FilePtrTy);
// Templates for summaries that are reused by many functions.
- auto Getc = [&]() {
- return Summary(ArgTypes{FilePtrTy}, RetType{IntTy}, NoEvalCall)
- .Case({ReturnValueCondition(WithinRange,
- {{EOFv, EOFv}, {0, UCharRangeMax}})});
- };
auto Read = [&](RetType R, RangeInt Max) {
return Summary(ArgTypes{Irrelevant, Irrelevant, SizeTy}, RetType{R},
NoEvalCall)
.Case({ReturnValueCondition(LessThanOrEq, ArgNo(2)),
ReturnValueCondition(WithinRange, Range(-1, Max))});
};
- auto Fread = [&]() {
- return Summary(
- ArgTypes{VoidPtrRestrictTy, SizeTy, SizeTy, FilePtrRestrictTy},
- RetType{SizeTy}, NoEvalCall)
- .Case({
- ReturnValueCondition(LessThanOrEq, ArgNo(2)),
- })
- .ArgConstraint(NotNull(ArgNo(0)));
- };
- auto Fwrite = [&]() {
- return Summary(ArgTypes{ConstVoidPtrRestrictTy, SizeTy, SizeTy,
- FilePtrRestrictTy},
- RetType{SizeTy}, NoEvalCall)
- .Case({
- ReturnValueCondition(LessThanOrEq, ArgNo(2)),
- })
- .ArgConstraint(NotNull(ArgNo(0)));
- };
auto Getline = [&](RetType R, RangeInt Max) {
return Summary(ArgTypes{Irrelevant, Irrelevant, Irrelevant}, RetType{R},
NoEvalCall)
@@ -1283,19 +1260,45 @@
0U, WithinRange, {{EOFv, EOFv}, {0, UCharRangeMax}})));
// The getc() family of functions that returns either a char or an EOF.
- addToFunctionSummaryMap("getc", Getc());
- addToFunctionSummaryMap("fgetc", Getc());
+ addToFunctionSummaryMap(
+ {"getc", "fgetc"}, Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
+ Summary(NoEvalCall)
+ .Case({ReturnValueCondition(WithinRange,
+ {{EOFv, EOFv}, {0, UCharRangeMax}})}));
addToFunctionSummaryMap(
"getchar", Summary(ArgTypes{}, RetType{IntTy}, NoEvalCall)
.Case({ReturnValueCondition(
WithinRange, {{EOFv, EOFv}, {0, UCharRangeMax}})}));
// read()-like functions that never return more than buffer size.
- addToFunctionSummaryMap("fread", Fread());
- addToFunctionSummaryMap("fwrite", Fwrite());
+ auto FreadSummary =
+ Summary(NoEvalCall)
+ .Case({
+ ReturnValueCondition(LessThanOrEq, ArgNo(2)),
+ })
+ .ArgConstraint(NotNull(ArgNo(0)))
+ .ArgConstraint(NotNull(ArgNo(3)))
+ .ArgConstraint(BufferSize(/*Buffer=*/ArgNo(0), /*BufSize=*/ArgNo(1),
+ /*BufSizeMultiplier=*/ArgNo(2)));
+
+ // size_t fread(void *restrict ptr, size_t size, size_t nitems,
+ // FILE *restrict stream);
+ addToFunctionSummaryMap(
+ "fread",
+ Signature(ArgTypes{VoidPtrRestrictTy, SizeTy, SizeTy, FilePtrRestrictTy},
+ RetType{SizeTy}),
+ FreadSummary);
+ // size_t fwrite(const void *restrict ptr, size_t size, size_t nitems,
+ // FILE *restrict stream);
+ addToFunctionSummaryMap("fwrite",
+ Signature(ArgTypes{ConstVoidPtrRestrictTy, SizeTy,
+ SizeTy, FilePtrRestrictTy},
+ RetType{SizeTy}),
+ FreadSummary);
// We are not sure how ssize_t is defined on every platform, so we
// provide three variants that should cover common cases.
+ // FIXME Use lookupTy("ssize_t") instead of the `Read` lambda.
// FIXME these are actually defined by POSIX and not by the C standard, we
// should handle them together with the rest of the POSIX functions.
addToFunctionSummaryMap("read", {Read(IntTy, IntMax), Read(LongTy, LongMax),
@@ -1304,11 +1307,13 @@
Read(LongLongTy, LongLongMax)});
// getline()-like functions either fail or read at least the delimiter.
+ // FIXME Use lookupTy("ssize_t") instead of the `Getline` lambda.
// FIXME these are actually defined by POSIX and not by the C standard, we
// should handle them together with the rest of the POSIX functions.
addToFunctionSummaryMap("getline",
{Getline(IntTy, IntMax), Getline(LongTy, LongMax),
Getline(LongLongTy, LongLongMax)});
+ // FIXME getdelim's signature is different than getline's!
addToFunctionSummaryMap("getdelim",
{Getline(IntTy, IntMax), Getline(LongTy, LongMax),
Getline(LongLongTy, LongLongMax)});
diff --git a/clang/test/Analysis/Inputs/system-header-simulator.h b/clang/test/Analysis/Inputs/system-header-simulator.h
index a98546c..b72f45a 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator.h
@@ -46,8 +46,8 @@
FILE *tmpfile(void);
FILE *freopen(const char *pathname, const char *mode, FILE *stream);
int fclose(FILE *fp);
-size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
-size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
+size_t fread(void *restrict, size_t, size_t, FILE *restrict);
+size_t fwrite(const void *restrict, size_t, size_t, FILE *restrict);
int fputc(int ch, FILE *stream);
int fseek(FILE *__stream, long int __off, int __whence);
long int ftell(FILE *__stream);
diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c
index bef786a..7c00e78 100644
--- a/clang/test/Analysis/analyzer-enabled-checkers.c
+++ b/clang/test/Analysis/analyzer-enabled-checkers.c
@@ -6,11 +6,11 @@
// CHECK: OVERVIEW: Clang Static Analyzer Enabled Checkers List
// CHECK-EMPTY:
+// CHECK-NEXT: core.CallAndMessageModeling
// CHECK-NEXT: apiModeling.StdCLibraryFunctions
// CHECK-NEXT: apiModeling.TrustNonnull
// CHECK-NEXT: apiModeling.llvm.CastValue
// CHECK-NEXT: apiModeling.llvm.ReturnValue
-// CHECK-NEXT: core.CallAndMessageModeling
// CHECK-NEXT: core.CallAndMessage
// CHECK-NEXT: core.DivideZero
// CHECK-NEXT: core.DynamicTypePropagation
diff --git a/clang/test/Analysis/std-c-library-functions-arg-constraints.c b/clang/test/Analysis/std-c-library-functions-arg-constraints.c
index 28979ab..afc2ce2 100644
--- a/clang/test/Analysis/std-c-library-functions-arg-constraints.c
+++ b/clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -194,6 +194,22 @@
// bugpath-warning{{Function argument constraint is not satisfied}} \
// bugpath-note{{Function argument constraint is not satisfied}}
}
+typedef __WCHAR_TYPE__ wchar_t;
+// This is one test case for the ARR38-C SEI-CERT rule.
+void ARR38_C_F(FILE *file) {
+ enum { BUFFER_SIZE = 1024 };
+ wchar_t wbuf[BUFFER_SIZE]; // bugpath-note{{'wbuf' initialized here}}
+
+ const size_t size = sizeof(*wbuf);
+ const size_t nitems = sizeof(wbuf);
+
+ // The 3rd parameter should be the number of elements to read, not
+ // the size in bytes.
+ fread(wbuf, size, nitems, file); // \
+ // report-warning{{Function argument constraint is not satisfied}} \
+ // bugpath-warning{{Function argument constraint is not satisfied}} \
+ // bugpath-note{{Function argument constraint is not satisfied}}
+}
int __two_constrained_args(int, int);
void test_constraints_on_multiple_args(int x, int y) {
diff --git a/clang/test/Analysis/std-c-library-functions-vs-stream-checker.c b/clang/test/Analysis/std-c-library-functions-vs-stream-checker.c
new file mode 100644
index 0000000..61106f1
--- /dev/null
+++ b/clang/test/Analysis/std-c-library-functions-vs-stream-checker.c
@@ -0,0 +1,58 @@
+// Check the case when only the StreamChecker is enabled.
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core,alpha.unix.Stream \
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config eagerly-assume=false \
+// RUN: -triple x86_64-unknown-linux \
+// RUN: -verify=stream
+
+// Check the case when only the StdLibraryFunctionsChecker is enabled.
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config eagerly-assume=false \
+// RUN: -triple x86_64-unknown-linux \
+// RUN: -verify=stdLib 2>&1 | FileCheck %s
+
+// Check the case when both the StreamChecker and the
+// StdLibraryFunctionsChecker are enabled.
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core,alpha.unix.Stream \
+// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config eagerly-assume=false \
+// RUN: -triple x86_64-unknown-linux \
+// RUN: -verify=both 2>&1 | FileCheck %s
+
+// Verify that the summaries are loaded when the StdLibraryFunctionsChecker is
+// enabled.
+// CHECK: Loaded summary for: int getchar()
+// CHECK-NEXT: Loaded summary for: unsigned long fread(void *restrict, size_t, size_t, FILE *restrict)
+// CHECK-NEXT: Loaded summary for: unsigned long fwrite(const void *restrict, size_t, size_t, FILE *restrict)
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+
+void test_fread_fwrite(FILE *fp, int *buf) {
+ fp = fopen("foo", "r");
+ if (!fp)
+ return;
+ size_t x = fwrite(buf, sizeof(int), 10, fp);
+
+ clang_analyzer_eval(x <= 10); // \
+ // stream-warning{{TRUE}} \
+ // stdLib-warning{{TRUE}} \
+ // both-warning{{TRUE}} \
+
+ clang_analyzer_eval(x == 10); // \
+ // stream-warning{{TRUE}} \
+ // stream-warning{{FALSE}} \
+ // stdLib-warning{{UNKNOWN}} \
+ // both-warning{{TRUE}} \
+ // both-warning{{FALSE}}
+
+ fclose(fp);
+}