martong created this revision.
martong added reviewers: steakhal, balazske, Szelethus, NoQ, vsavchenko.
Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, gamesh411,
dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet,
baloghadamsoftware, xazax.hun, whisperity.
Herald added a project: clang.
martong requested review of this revision.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D87081
Files:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
clang/test/Analysis/std-c-library-functions-arg-constraints.c
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===================================================================
--- clang/test/Analysis/std-c-library-functions-arg-constraints.c
+++ 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) {
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -989,6 +989,12 @@
for (const Summary &S : Summaries)
operator()(Name, S);
}
+ // Add the same summary for different names with the Signature explicitly
+ // given.
+ void operator()(std::vector<StringRef> Names, Signature Sign, Summary Sum) {
+ for (StringRef Name : Names)
+ operator()(Name, Sign, Sum);
+ }
} addToFunctionSummaryMap(ACtx, FunctionSummaryMap, DisplayLoadedSummaries);
// Below are helpers functions to create the summaries.
@@ -1030,35 +1036,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)
@@ -1223,19 +1206,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),
@@ -1244,11 +1253,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 differant than getline's!
addToFunctionSummaryMap("getdelim",
{Getline(IntTy, IntMax), Getline(LongTy, LongMax),
Getline(LongLongTy, LongLongMax)});
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
