https://github.com/gamesh411 created https://github.com/llvm/llvm-project/pull/186802
It turns out, that some checks for cstring functions happened as a side effect of other checks. For example, whether the arguments to memcpy were uninitialized happened during buffer overflow checking. The way this was implemented is that if alpha.unix.cstring.OutOfBounds was disabled, alpha.unix.cstring.UninitializedRead couldn't emit any warnings. It turns out that major modeling steps are early-exited if a certain checker is disabled! This patch moved the early returns to the report emission parts -- modeling still happens, only the bug report construction is omitted. This would mean that if we find a fatal error (like buffer overflow) we _should_ stop analysis even if we don't emit a warning (thats a part of doing modeling), but I decided against implementing that. One hurdle is that CStringChecker is a dependency of MallocChecker, and the current tests rely on the CStringChecker _not_ terminating execution paths prematurely. Considering that the checkers that would do that are in alpha anyways, this doesn't seem to be an urgent step immediately. I added FIXMEs to all tests would have failed if the patch sank the analysis at the fatal cstring function call, but didn't. I also added a new test case for buffers overlapping, but not being quite equal. Original Author: Kristóf Umann <[email protected]> Co-Author: Endre Fülöp <[email protected]> From 73957c5cc93dff86cfed689696bea91367e9fec8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krist=C3=B3f=20Umann?= <[email protected]> Date: Tue, 22 Oct 2024 11:18:37 +0200 Subject: [PATCH] [analyzer] Untangle subcheckers of CStringChecker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It turns out, that some checks for cstring functions happened as a side effect of other checks. For example, whether the arguments to memcpy were uninitialized happened during buffer overflow checking. The way this was implemented is that if alpha.unix.cstring.OutOfBounds was disabled, alpha.unix.cstring.UninitializedRead couldn't emit any warnings. It turns out that major modeling steps are early-exited if a certain checker is disabled! This patch moved the early returns to the report emission parts -- modeling still happens, only the bug report construction is omitted. This would mean that if we find a fatal error (like buffer overflow) we _should_ stop analysis even if we don't emit a warning (thats a part of doing modeling), but I decided against implementing that. One hurdle is that CStringChecker is a dependency of MallocChecker, and the current tests rely on the CStringChecker _not_ terminating execution paths prematurely. Considering that the checkers that would do that are in alpha anyways, this doesn't seem to be an urgent step immediately. I added FIXMEs to all tests would have failed if the patch sank the analysis at the fatal cstring function call, but didn't. I also added a new test case for buffers overlapping, but not being quite equal. Original Author: Kristóf Umann <[email protected]> Co-Author: Endre Fülöp <[email protected]> --- .../Checkers/CStringChecker.cpp | 51 +++++++++++------ clang/test/Analysis/bstring.cpp | 55 +++++++++++++++++-- clang/test/Analysis/malloc.c | 11 ++++ 3 files changed, 94 insertions(+), 23 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 144411495f5a1..4fccdf8a0678d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -576,8 +576,11 @@ ProgramStateRef CStringChecker::CheckLocation(CheckerContext &C, auto [StInBound, StOutBound] = state->assumeInBoundDual(*Idx, Size); if (StOutBound && !StInBound) { + // FIXME: We detected a fatal error here, we should stop analysis even if we + // chose not to emit a report here. However, as long as our out-of-bounds + // checker is in alpha, lets just pretend nothing happened. if (!OutOfBounds.isEnabled()) - return nullptr; + return state; ErrorMessage Message = createOutOfBoundErrorMsg(CurrentFunctionDescription, Access); @@ -610,10 +613,6 @@ CStringChecker::CheckBufferAccess(CheckerContext &C, ProgramStateRef State, if (!State) return nullptr; - // If out-of-bounds checking is turned off, skip the rest. - if (!OutOfBounds.isEnabled()) - return State; - SVal BufStart = svalBuilder.evalCast(BufVal, PtrTy, Buffer.Expression->getType()); @@ -661,9 +660,6 @@ ProgramStateRef CStringChecker::CheckOverlap(CheckerContext &C, SizeArgExpr Size, AnyArgExpr First, AnyArgExpr Second, CharKind CK) const { - if (!BufferOverlap.isEnabled()) - return state; - // Do a simple check for overlap: if the two arguments are from the same // buffer, see if the end of the first is greater than the start of the second // or vice versa. @@ -702,9 +698,15 @@ ProgramStateRef CStringChecker::CheckOverlap(CheckerContext &C, state->assume(svalBuilder.evalEQ(state, *firstLoc, *secondLoc)); if (stateTrue && !stateFalse) { - // If the values are known to be equal, that's automatically an overlap. - emitOverlapBug(C, stateTrue, First.Expression, Second.Expression); - return nullptr; + if (BufferOverlap.isEnabled()) { + // If the values are known to be equal, that's automatically an overlap. + emitOverlapBug(C, stateTrue, First.Expression, Second.Expression); + return nullptr; + } + // FIXME: We detected a fatal error here, we should stop analysis even if we + // chose not to emit a report here. However, as long as our overlap checker + // is in alpha, lets just pretend nothing happened. + return state; } // assume the two expressions are not equal. @@ -769,8 +771,14 @@ ProgramStateRef CStringChecker::CheckOverlap(CheckerContext &C, if (stateTrue && !stateFalse) { // Overlap! - emitOverlapBug(C, stateTrue, First.Expression, Second.Expression); - return nullptr; + if (BufferOverlap.isEnabled()) { + emitOverlapBug(C, stateTrue, First.Expression, Second.Expression); + return nullptr; + } + // FIXME: We detected a fatal error here, we should stop analysis even if we + // chose not to emit a report here. However, as long as our overlap checker + // is in alpha, lets just pretend nothing happened. + return state; } // assume the two expressions don't overlap. @@ -779,7 +787,10 @@ ProgramStateRef CStringChecker::CheckOverlap(CheckerContext &C, } void CStringChecker::emitOverlapBug(CheckerContext &C, ProgramStateRef state, - const Stmt *First, const Stmt *Second) const { + const Stmt *First, + const Stmt *Second) const { + assert(BufferOverlap.isEnabled() && + "Can't emit from a checker that is not enabled!"); ExplodedNode *N = C.generateErrorNode(state); if (!N) return; @@ -795,6 +806,8 @@ void CStringChecker::emitOverlapBug(CheckerContext &C, ProgramStateRef state, void CStringChecker::emitNullArgBug(CheckerContext &C, ProgramStateRef State, const Stmt *S, StringRef WarningMsg) const { + assert(NullArg.isEnabled() && + "Can't emit from a checker that is not enabled!"); if (ExplodedNode *N = C.generateErrorNode(State)) { auto Report = std::make_unique<PathSensitiveBugReport>(NullArg, WarningMsg, N); @@ -809,6 +822,8 @@ void CStringChecker::emitUninitializedReadBug(CheckerContext &C, ProgramStateRef State, const Expr *E, const MemRegion *R, StringRef Msg) const { + assert(UninitializedRead.isEnabled() && + "Can't emit from a checker that is not enabled!"); if (ExplodedNode *N = C.generateErrorNode(State)) { auto Report = std::make_unique<PathSensitiveBugReport>(UninitializedRead, Msg, N); @@ -824,6 +839,8 @@ void CStringChecker::emitUninitializedReadBug(CheckerContext &C, void CStringChecker::emitOutOfBoundsBug(CheckerContext &C, ProgramStateRef State, const Stmt *S, StringRef WarningMsg) const { + assert(OutOfBounds.isEnabled() && + "Can't emit from a checker that is not enabled!"); if (ExplodedNode *N = C.generateErrorNode(State)) { // FIXME: It would be nice to eventually make this diagnostic more clear, // e.g., by referencing the original declaration or by saying *why* this @@ -838,6 +855,8 @@ void CStringChecker::emitOutOfBoundsBug(CheckerContext &C, void CStringChecker::emitNotCStringBug(CheckerContext &C, ProgramStateRef State, const Stmt *S, StringRef WarningMsg) const { + assert(NotNullTerm.isEnabled() && + "Can't emit from a checker that is not enabled!"); if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) { auto Report = std::make_unique<PathSensitiveBugReport>(NotNullTerm, WarningMsg, N); @@ -851,10 +870,6 @@ ProgramStateRef CStringChecker::checkAdditionOverflow(CheckerContext &C, ProgramStateRef state, NonLoc left, NonLoc right) const { - // If out-of-bounds checking is turned off, skip the rest. - if (!OutOfBounds.isEnabled()) - return state; - // If a previous check has failed, propagate the failure. if (!state) return nullptr; diff --git a/clang/test/Analysis/bstring.cpp b/clang/test/Analysis/bstring.cpp index 9c30bef15d407..385e7043053e2 100644 --- a/clang/test/Analysis/bstring.cpp +++ b/clang/test/Analysis/bstring.cpp @@ -1,8 +1,43 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s -// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s -// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,unix.Malloc,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s -// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,unix.Malloc,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s -// RUN: %clang_analyze_cc1 -DSUPPRESS_OUT_OF_BOUND -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring.BufferOverlap,unix.cstring.NotNullTerminated,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s +// RUN: %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=unix.cstring \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=alpha.unix.cstring \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false + +// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=unix.cstring \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=alpha.unix.cstring \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false + +// RUN: %clang_analyze_cc1 -verify %s -DVARIANT \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=unix.cstring \ +// RUN: -analyzer-checker=alpha.unix.cstring \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false + +// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS -DVARIANT \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=unix.cstring \ +// RUN: -analyzer-checker=alpha.unix.cstring \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false + +// RUN: %clang_analyze_cc1 -verify %s -DSUPPRESS_OUT_OF_BOUND \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=unix.cstring \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=alpha.unix.cstring.BufferOverlap \ +// RUN: -analyzer-checker=unix.cstring.NotNullTerminated \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false #include "Inputs/system-header-simulator-cxx.h" #include "Inputs/system-header-simulator-for-malloc.h" @@ -103,6 +138,8 @@ void memset1_inheritance() { #ifdef SUPPRESS_OUT_OF_BOUND void memset2_inheritance_field() { Derived d; + // FIXME: The analyzer should stop analysis after memset. The argument to + // sizeof should be Derived::d_mem. memset(&d.d_mem, 0, sizeof(Derived)); clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}} clang_analyzer_eval(d.d_mem == 0); // expected-warning{{UNKNOWN}} @@ -110,6 +147,8 @@ void memset2_inheritance_field() { void memset3_inheritance_field() { Derived d; + // FIXME: The analyzer should stop analysis after memset. The argument to + // sizeof should be Derived::b_mem. memset(&d.b_mem, 0, sizeof(Derived)); clang_analyzer_eval(d.b_mem == 0); // expected-warning{{TRUE}} clang_analyzer_eval(d.d_mem == 0); // expected-warning{{TRUE}} @@ -176,6 +215,8 @@ class DerivedVirtual : public BaseVirtual { #ifdef SUPPRESS_OUT_OF_BOUND void memset8_virtual_inheritance_field() { DerivedVirtual d; + // FIXME: The analyzer should stop analysis after memset. The argument to + // sizeof should be Derived::b_mem. memset(&d.b_mem, 0, sizeof(Derived)); clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}} clang_analyzer_eval(d.d_mem == 0); // expected-warning{{UNKNOWN}} @@ -188,6 +229,10 @@ void memset1_new_array() { int *array = new int[10]; memset(array, 0, 10 * sizeof(int)); clang_analyzer_eval(array[2] == 0); // expected-warning{{TRUE}} + // FIXME: The analyzer should stop analysis after memset. Maybe the intent of + // this test was to test for this as a desired behaviour, but it shouldn't be, + // going out-of-bounds with memset is a fatal error, even if we decide not to + // report it. memset(array + 1, 'a', 10 * sizeof(9)); clang_analyzer_eval(array[2] == 0); // expected-warning{{UNKNOWN}} delete[] array; diff --git a/clang/test/Analysis/malloc.c b/clang/test/Analysis/malloc.c index 92b47bc3b5e9a..1492659d8a128 100644 --- a/clang/test/Analysis/malloc.c +++ b/clang/test/Analysis/malloc.c @@ -870,12 +870,23 @@ void doNotInvalidateWhenPassedToSystemCalls(char *s) { strlen(p); strcpy(p, s); strcpy(s, p); + // FIXME: We should stop analysis here, even if we emit no warnings, since + // overlapping buffers for strycpy is a fatal error. strcpy(p, p); memcpy(p, s, 1); memcpy(s, p, 1); memcpy(p, p, 1); } // expected-warning {{leak}} +void doNotInvalidateWhenPassedToSystemCalls2(char *s) { + char *p = malloc(12); + // FIXME: We should stop analysis here, even if we emit no warnings, since + // overlapping buffers for strycpy is a fatal error. + int a[4] = {0}; + memcpy(a+2, a+1, 8); + (void)p; +} // expected-warning {{leak}} + // Treat source buffer contents as escaped. void escapeSourceContents(char *s) { char *p = malloc(12); _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
