From a012bc4c42e4408a18e4c4d67306b79c576df961 Mon Sep 17 00:00:00 2001 From: Gabor Marton Date: Thu, 3 Sep 2020 13:23:49 +0200 Subject: [PATCH] [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 --- .../clang/StaticAnalyzer/Checkers/Checkers.td | 3 ++ .../Checkers/StdLibraryFunctionsChecker.cpp | 59 ++++++++++++---------- .../test/Analysis/Inputs/system-header-simulator.h | 4 +- clang/test/Analysis/analyzer-enabled-checkers.c | 2 +- .../std-c-library-functions-arg-constraints.c | 16 ++++++ .../std-c-library-functions-vs-stream-checker.c | 58 +++++++++++++++++++++ 6 files changed, 112 insertions(+), 30 deletions(-) create mode 100644 clang/test/Analysis/std-c-library-functions-vs-stream-checker.c 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 @@ 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 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 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 *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 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 @@ 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 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); +} -- 2.7.4