llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Balázs Kéri (balazske) <details> <summary>Changes</summary> New option is added to the checker to create branches with null return value from memory allocations (off by default). --- Full diff: https://github.com/llvm/llvm-project/pull/205371.diff 4 Files Affected: - (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+8-1) - (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+70-5) - (modified) clang/test/Analysis/analyzer-config.c (+1) - (added) clang/test/Analysis/malloc-failure.c (+76) ``````````diff diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index eca2afbe340a9..0249d292e51cd 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -512,7 +512,14 @@ def DynamicMemoryModeling: Checker<"DynamicMemoryModeling">, "NoStoreFuncVisitor.", "true", Released, - Hide> + Hide>, + CmdLineOption<Boolean, + "ModelAllocationFailure", + "At allocation functions, add extra execution branches for " + "failed memory allocation (function returns null value when " + "size is non-null) if applicable.", + "false", + Released> ]>, Dependencies<[CStringModeling]>, Documentation<NotDocumented>, diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 25ad8959cac5f..8c096a5097cbf 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -401,6 +401,14 @@ class MallocChecker bool ShouldRegisterNoOwnershipChangeVisitor = false; + /// Add extra branches for allocation failure. Generally a return value of an + /// allocation function is not constrained to be null or non-null and + /// information about a later null pointer access can be lost. When failure + /// branches are added, they contain the constrained null pointer return value + /// and allow detection of a null pointer access if the result of an + /// allocation is not checked for null. + bool ModelAllocationFailure = false; + // This checker family implements many bug types and frontends, and several // bug types are shared between multiple frontends, so most of the frontends // are declared with the helper class DynMemFrontend. @@ -466,6 +474,7 @@ class MallocChecker CHECK_FN(checkFree) CHECK_FN(checkIfNameIndex) CHECK_FN(checkBasicAlloc) + CHECK_FN(checkBasicAllocMayFail) CHECK_FN(checkKernelMalloc) CHECK_FN(checkCalloc) CHECK_FN(checkAlloca) @@ -530,7 +539,7 @@ class MallocChecker }; CallDescriptionMap<CheckFn> AllocatingMemFnMap{ - {{CDM::CLibrary, {"malloc"}, 1}, &MallocChecker::checkBasicAlloc}, + {{CDM::CLibrary, {"malloc"}, 1}, &MallocChecker::checkBasicAllocMayFail}, {{CDM::CLibrary, {"malloc"}, 3}, &MallocChecker::checkKernelMalloc}, {{CDM::CLibrary, {"calloc"}, 2}, &MallocChecker::checkCalloc}, {{CDM::CLibrary, {"valloc"}, 1}, &MallocChecker::checkBasicAlloc}, @@ -538,7 +547,7 @@ class MallocChecker {{CDM::CLibrary, {"strdup"}, 1}, &MallocChecker::checkStrdup}, {{CDM::CLibrary, {"_strdup"}, 1}, &MallocChecker::checkStrdup}, {{CDM::CLibrary, {"kmalloc"}, 2}, &MallocChecker::checkKernelMalloc}, - {{CDM::CLibrary, {"if_nameindex"}, 1}, &MallocChecker::checkIfNameIndex}, + {{CDM::CLibrary, {"if_nameindex"}, 0}, &MallocChecker::checkIfNameIndex}, {{CDM::CLibrary, {"wcsdup"}, 1}, &MallocChecker::checkStrdup}, {{CDM::CLibrary, {"_wcsdup"}, 1}, &MallocChecker::checkStrdup}, {{CDM::CLibrary, {"g_malloc"}, 1}, &MallocChecker::checkBasicAlloc}, @@ -663,6 +672,11 @@ class MallocChecker SVal Init, ProgramStateRef State, AllocationFamily Family) const; + [[nodiscard]] ProgramStateRef FailedAlloc(CheckerContext &C, + const CallEvent &Call, + int SizeArgI1, int SizeArgI2, + ProgramStateRef State) const; + // Check if this malloc() for special flags. At present that means M_ZERO or // __GFP_ZERO (in which case, treat it like calloc). [[nodiscard]] std::optional<ProgramStateRef> @@ -1354,6 +1368,17 @@ void MallocChecker::checkBasicAlloc(ProgramStateRef State, C.addTransition(State); } +void MallocChecker::checkBasicAllocMayFail(ProgramStateRef State, + const CallEvent &Call, + CheckerContext &C) const { + C.addTransition(FailedAlloc(C, Call, 0, -1, State)); + + State = MallocMemAux(C, Call, Call.getArgExpr(0), UndefinedVal(), State, + AllocationFamily(AF_Malloc)); + State = ProcessZeroAllocCheck(C, Call, 0, State); + C.addTransition(State); +} + void MallocChecker::checkKernelMalloc(ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const { @@ -1389,14 +1414,18 @@ static bool isGRealloc(const CallEvent &Call) { void MallocChecker::checkRealloc(ProgramStateRef State, const CallEvent &Call, CheckerContext &C, bool ShouldFreeOnFail) const { + bool StandardRealloc = isStandardRealloc(Call); // Ignore calls to functions whose type does not match the expected type of // either the standard realloc or g_realloc from GLib. // FIXME: Should we perform this kind of checking consistently for each // function? If yes, then perhaps extend the `CallDescription` interface to // handle this. - if (!isStandardRealloc(Call) && !isGRealloc(Call)) + if (!StandardRealloc && !isGRealloc(Call)) return; + if (StandardRealloc) + C.addTransition(FailedAlloc(C, Call, 1, -1, State)); + State = ReallocMemAux(C, Call, ShouldFreeOnFail, State, AllocationFamily(AF_Malloc)); State = ProcessZeroAllocCheck(C, Call, 1, State); @@ -1405,6 +1434,8 @@ void MallocChecker::checkRealloc(ProgramStateRef State, const CallEvent &Call, void MallocChecker::checkCalloc(ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const { + C.addTransition(FailedAlloc(C, Call, 0, 1, State)); + State = CallocMem(C, Call, State); State = ProcessZeroAllocCheck(C, Call, 0, State); State = ProcessZeroAllocCheck(C, Call, 1, State); @@ -1434,20 +1465,23 @@ void MallocChecker::checkStrdup(ProgramStateRef State, const CallEvent &Call, const auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr()); if (!CE) return; + + C.addTransition(FailedAlloc(C, Call, -1, -1, State)); + State = MallocMemAux(C, Call, UnknownVal(), UnknownVal(), State, AllocationFamily(AF_Malloc)); - C.addTransition(State); } void MallocChecker::checkIfNameIndex(ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const { + C.addTransition(FailedAlloc(C, Call, -1, -1, State)); + // Should we model this differently? We can allocate a fixed number of // elements with zeros in the last one. State = MallocMemAux(C, Call, UnknownVal(), UnknownVal(), State, AllocationFamily(AF_IfNameIndex)); - C.addTransition(State); } @@ -2033,6 +2067,7 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C, SVal RetVal = State->getSVal(CE, C.getStackFrame()); // Fill the region with the initialization value. + // FIXME: Why use stack frame of the predecessor? State = State->bindDefaultInitial(RetVal, Init, SF); // If Size is somehow undefined at this point, this line prevents a crash. @@ -2048,6 +2083,33 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C, return MallocUpdateRefState(C, CE, State, Family); } +ProgramStateRef MallocChecker::FailedAlloc(CheckerContext &C, + const CallEvent &Call, int SizeArgI1, + int SizeArgI2, + ProgramStateRef State) const { + if (!State || !ModelAllocationFailure) + return nullptr; + + auto AssumeArgNonZero = [&Call](ProgramStateRef State, + int ArgI) -> ProgramStateRef { + if (!State || ArgI < 0) + return State; + auto DefArgVal = Call.getArgSVal(ArgI).getAs<DefinedOrUnknownSVal>(); + if (!DefArgVal) + return nullptr; + return State->assume(*DefArgVal, true); + }; + + State = AssumeArgNonZero(State, SizeArgI1); + State = AssumeArgNonZero(State, SizeArgI2); + if (!State) + return nullptr; + + auto RetVal = State->getSVal(Call.getOriginExpr(), C.getStackFrame()) + .castAs<DefinedOrUnknownSVal>(); + return State->assume(RetVal, false); +} + static ProgramStateRef MallocUpdateRefState(CheckerContext &C, const Expr *E, ProgramStateRef State, AllocationFamily Family, @@ -4201,6 +4263,9 @@ void ento::registerDynamicMemoryModeling(CheckerManager &Mgr) { Chk->ShouldRegisterNoOwnershipChangeVisitor = Mgr.getAnalyzerOptions().getCheckerBooleanOption( DMMName, "AddNoOwnershipChangeNotes"); + Chk->ModelAllocationFailure = + Mgr.getAnalyzerOptions().getCheckerBooleanOption( + DMMName, "ModelAllocationFailure"); } bool ento::shouldRegisterDynamicMemoryModeling(const CheckerManager &mgr) { diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c index 04dc8c24421bc..a952c64aab0f4 100644 --- a/clang/test/Analysis/analyzer-config.c +++ b/clang/test/Analysis/analyzer-config.c @@ -135,6 +135,7 @@ // CHECK-NEXT: track-conditions = true // CHECK-NEXT: track-conditions-debug = false // CHECK-NEXT: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes = true +// CHECK-NEXT: unix.DynamicMemoryModeling:ModelAllocationFailure = false // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false // CHECK-NEXT: unix.Errno:AllowErrnoReadOutsideConditionExpressions = true // CHECK-NEXT: unix.StdCLibraryFunctions:DisplayLoadedSummaries = false diff --git a/clang/test/Analysis/malloc-failure.c b/clang/test/Analysis/malloc-failure.c new file mode 100644 index 0000000000000..aea58dad470fa --- /dev/null +++ b/clang/test/Analysis/malloc-failure.c @@ -0,0 +1,76 @@ +// RUN: %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core,unix.DynamicMemoryModeling \ +// RUN: -analyzer-config unix.DynamicMemoryModeling:ModelAllocationFailure=true +// RUN: %clang_analyze_cc1 -verify=nofailurebranch %s \ +// RUN: -analyzer-checker=core,unix.DynamicMemoryModeling \ +// RUN: -analyzer-config unix.DynamicMemoryModeling:ModelAllocationFailure=false + +// nofailurebranch-no-diagnostics + +typedef __typeof(sizeof(int)) size_t; +void *malloc(size_t); +void *alloca(size_t); +void *valloc(size_t); +void free(void *); +void *realloc(void *ptr, size_t size); +void *calloc(size_t nmemb, size_t size); +char *strdup(const char *s); +struct if_nameindex { char x; }; +struct if_nameindex *if_nameindex(void); +void if_freenameindex(struct if_nameindex *ptr); + +void test_malloc(size_t s) { + char *p = malloc(s); + if (s > 0) { + *p = 1; //expected-warning{{Dereference of null pointer}} + } else { + *p = 1; + } + free(p); +} + +void test_calloc(size_t n, size_t s) { + char *p = calloc(n, s); + if (n > 0 && s > 0) { + *p = 1; //expected-warning{{Dereference of null pointer}} + } else { + *p = 1; + } + free(p); +} + +void test_valloc(size_t s) { + int *p = valloc(s); + *p = 1; //no-warning + free(p); +} + +void test_alloca(size_t s) { + int *p = alloca(s); + *p = 1; //no-warning +} + +void test_realloc(size_t s) { + char *p = malloc(10); + if (!p) + return; + p = realloc(p, s); + if (s > 0) { + *p = 1; //expected-warning{{Dereference of null pointer}} + } else { + *p = 1; + } + free(p); +} + +void test_strdup() { + char *p = strdup("abcd"); + *p = 1; //expected-warning{{Dereference of null pointer}} + free(p); +} + +void test_ifnameindex() { + struct if_nameindex *p = if_nameindex(); + p->x = 1; //expected-warning{{dereference of a null pointer}} + if_freenameindex(p); +} `````````` </details> https://github.com/llvm/llvm-project/pull/205371 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
