[clang] a012bc4 - [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite
Gabor Marton via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 15 07:36:45 PDT 2020
Author: Gabor Marton
Date: 2020-09-15T16:35:39+02:00
New Revision: a012bc4c42e4408a18e4c4d67306b79c576df961
URL: https://github.com/llvm/llvm-project/commit/a012bc4c42e4408a18e4c4d67306b79c576df961
DIFF: https://github.com/llvm/llvm-project/commit/a012bc4c42e4408a18e4c4d67306b79c576df961.diff
LOG: [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
Added:
clang/test/Analysis/std-c-library-functions-vs-stream-checker.c
Modified:
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
clang/test/Analysis/Inputs/system-header-simulator.h
clang/test/Analysis/analyzer-enabled-checkers.c
clang/test/Analysis/std-c-library-functions-arg-constraints.c
Removed:
################################################################################
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index a61af45231348..cbc048ba74c42 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -349,6 +349,9 @@ let ParentPackage = APIModeling in {
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 f5ad80950ef11..45711cad56337 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -1090,35 +1090,12 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
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 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
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 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
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
diff erent 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 a98546c7056c9..b72f45a9b0e55 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator.h
@@ -46,8 +46,8 @@ FILE *fopen(const char *path, const char *mode);
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 bef786a1a59b6..7c00e78c16acd 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 28979abd43b58..afc2ce28efc62 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 @@ void test_notnull_symbolic2(FILE *fp, int *buf) {
// 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 0000000000000..61106f1f8d6bc
--- /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);
+}
More information about the cfe-commits
mailing list