https://github.com/relaxcn updated https://github.com/llvm/llvm-project/pull/237901
>From 1dd04bcc2d5340238d347fb88c64949050809383 Mon Sep 17 00:00:00 2001 From: relaxcn <[email protected]> Date: Wed, 28 Oct 2026 18:17:10 +0800 Subject: [PATCH] [analyzer] Fix suppression lost through returns ReturnVisitor used one flag for two decisions that are not the same thing: whether the null return invalidates the report, and whether the tracked expression may still use the caller's false-positive suppression. With suppress-null-return-paths=false both answers became no, so an inlined defensive check stopped suppressing a null dereference reached through the return value, and we reported a false positive. I split the flag. TrackWithFPSuppression is what gets passed to the nested tracking; NullReturnSuppressesReport only decides whether the return is to blame for the report. The option now controls just the second one. The argument path still passes the null-return decision down, as it did before. I left that alone because changing it is a separate question. Tests cover a pointer returned from a getter and from a function-like macro, both checked before the dereference. --- .../Core/BugReporterVisitors.cpp | 51 ++++++++++++------- .../inlining/false-positive-suppression.c | 32 ++++++++++++ 2 files changed, 65 insertions(+), 18 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 5c6c2a4612b69b..6ab52af152bdb6 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -906,17 +906,28 @@ class ReturnVisitor : public TrackingBugReporterVisitor { Satisfied } Mode = Initial; - bool EnableNullFPSuppression; + /// Whether the null value returned by the callee suppresses the report + /// (suppress-null-return-paths). + bool NullReturnSuppressesReport; + /// Whether tracking the returned expression may still apply the caller's + /// false-positive suppression (inlined defensive checks, macro null + /// returns). Unlike the flag above, this one is handed to the nested + /// tracking instead of deciding the fate of this report. + bool TrackWithFPSuppression; + /// Whether this visitor may still suppress the report. Cleared when the null + /// came from an argument, so the callee's null return is not to blame. bool ShouldInvalidate = true; AnalyzerOptions& Options; bugreporter::TrackingKind TKind; public: ReturnVisitor(TrackerRef ParentTracker, const StackFrame *Frame, - bool Suppressed, AnalyzerOptions &Options, - bugreporter::TrackingKind TKind) + bool NullReturnSuppresses, bool TrackSuppression, + AnalyzerOptions &Options, bugreporter::TrackingKind TKind) : TrackingBugReporterVisitor(ParentTracker), CalleeSF(Frame), - EnableNullFPSuppression(Suppressed), Options(Options), TKind(TKind) {} + NullReturnSuppressesReport(NullReturnSuppresses), + TrackWithFPSuppression(TrackSuppression), Options(Options), + TKind(TKind) {} static void *getTag() { static int Tag = 0; @@ -926,7 +937,8 @@ class ReturnVisitor : public TrackingBugReporterVisitor { void Profile(llvm::FoldingSetNodeID &ID) const override { ID.AddPointer(ReturnVisitor::getTag()); ID.AddPointer(CalleeSF); - ID.AddBoolean(EnableNullFPSuppression); + ID.AddBoolean(NullReturnSuppressesReport); + ID.AddBoolean(TrackWithFPSuppression); } PathDiagnosticPieceRef visitNodeInitial(const ExplodedNode *N, @@ -977,7 +989,7 @@ class ReturnVisitor : public TrackingBugReporterVisitor { RetE = RetE->IgnoreParenCasts(); // Let's track the return value. - getParentTracker().track(RetE, N, {TKind, EnableNullFPSuppression}); + getParentTracker().track(RetE, N, {TKind, TrackWithFPSuppression}); // Build an appropriate message based on the return value. SmallString<64> Msg; @@ -991,7 +1003,7 @@ class ReturnVisitor : public TrackingBugReporterVisitor { // If we have counter-suppression enabled, make sure we keep visiting // future nodes. We want to emit a path note as well, in case // the report is resurrected as valid later on. - if (EnableNullFPSuppression && + if (NullReturnSuppressesReport && Options.ShouldAvoidSuppressingNullArgumentPaths) Mode = MaybeUnsuppress; @@ -1091,7 +1103,7 @@ class ReturnVisitor : public TrackingBugReporterVisitor { continue; if (getParentTracker() - .track(ArgE, N, {TKind, EnableNullFPSuppression}) + .track(ArgE, N, {TKind, NullReturnSuppressesReport}) .FoundSomethingToTrack) ShouldInvalidate = false; @@ -1120,7 +1132,7 @@ class ReturnVisitor : public TrackingBugReporterVisitor { void finalizeVisitor(const ExplodedNode *, BugReporterContext &, PathSensitiveBugReport &BR) override { - if (EnableNullFPSuppression && ShouldInvalidate) + if (NullReturnSuppressesReport && ShouldInvalidate) BR.markInvalid(ReturnVisitor::getTag(), CalleeSF); } }; @@ -2427,18 +2439,21 @@ class InlinedFunctionCallHandler final : public ExpressionHandler { if (std::optional<Loc> LValue = RetVal.getAs<Loc>()) RetVal = State->getSVal(*LValue); - // See if the return value is NULL. If so, suppress the report. - AnalyzerOptions &Options = State->getAnalysisManager().options; + bool ReturnsNull = false; + if (std::optional<Loc> RetLoc = RetVal.getAs<Loc>()) + ReturnsNull = State->isNull(*RetLoc).isConstrainedTrue(); - bool EnableNullFPSuppression = false; - if (Opts.EnableNullFPSuppression && Options.ShouldSuppressNullReturnPaths) - if (std::optional<Loc> RetLoc = RetVal.getAs<Loc>()) - EnableNullFPSuppression = State->isNull(*RetLoc).isConstrainedTrue(); + // Keep the tracking permission separate from the decision to suppress the + // report: suppress-null-return-paths only governs the latter. + AnalyzerOptions &Options = State->getAnalysisManager().options; + bool TrackWithFPSuppression = Opts.EnableNullFPSuppression && ReturnsNull; + bool NullReturnSuppressesReport = + TrackWithFPSuppression && Options.ShouldSuppressNullReturnPaths; PathSensitiveBugReport &Report = getParentTracker().getReport(); - Report.addVisitor<ReturnVisitor>(&getParentTracker(), CalleeSF, - EnableNullFPSuppression, Options, - Opts.Kind); + Report.addVisitor<ReturnVisitor>( + &getParentTracker(), CalleeSF, NullReturnSuppressesReport, + TrackWithFPSuppression, Options, Opts.Kind); return {true}; } }; diff --git a/clang/test/Analysis/inlining/false-positive-suppression.c b/clang/test/Analysis/inlining/false-positive-suppression.c index cdc32ebf2a0ce3..43fc50a9a569b1 100644 --- a/clang/test/Analysis/inlining/false-positive-suppression.c +++ b/clang/test/Analysis/inlining/false-positive-suppression.c @@ -1,6 +1,7 @@ // RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-config suppress-null-return-paths=false -verify %s // RUN: %clang_analyze_cc1 -analyzer-checker=core -verify -DSUPPRESSED=1 %s // RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-config avoid-suppressing-null-argument-paths=true -DSUPPRESSED=1 -DNULL_ARGS=1 -verify %s +// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-config suppress-null-return-paths=false,suppress-inlined-defensive-checks=false -analyze-function=testInlineDefensiveCheckThroughReturn -verify=noidc %s int opaquePropertyCheck(void *object); int coin(void); @@ -93,6 +94,27 @@ int triggerDivZero (void) { return 5/y; // expected-warning {{Division by zero}} } +struct PointerWrapper { + int *ptr; +}; + +static int *getWrappedPointer(struct PointerWrapper *wrapper) { + return wrapper->ptr; +} + +static void checkWrappedPointer(struct PointerWrapper *wrapper) { + if (!wrapper->ptr) + return; + *wrapper->ptr = 1; +} + +// Defensive-check suppression must survive tracking through a getter even when +// null-return suppression is disabled. +int testInlineDefensiveCheckThroughReturn(struct PointerWrapper *wrapper) { + checkWrappedPointer(wrapper); + return *getWrappedPointer(wrapper); // noidc-warning {{Dereference of null pointer}} +} + // Treat a function-like macro similarly to an inlined function, so suppress // warnings along paths resulting from inlined checks. #define MACRO_WITH_CHECK(a) ( ((a) != 0) ? *a : 17) @@ -103,6 +125,16 @@ void testInlineCheckInMacro(int *p) { *p = 1; // no-warning } +static int *getPointerAfterMacroCheck(int *p) { + int i = MACRO_WITH_CHECK(p); + (void)i; + return p; +} + +int testInlineCheckInMacroThroughReturn(int *p) { + return *getPointerAfterMacroCheck(p); // no-warning +} + #define MACRO_WITH_NESTED_CHECK(a) ( { int j = MACRO_WITH_CHECK(a); j; } ) void testInlineCheckInNestedMacro(int *p) { int i = MACRO_WITH_NESTED_CHECK(p); _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
