Backport to 20.x "[clang][analyzer] Fix error path of builtin overflow (#136345)"

[pskrgag]

According to
https://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins,
result of builtin_*_overflow functions will be initialized even in case
of overflow. Align analyzer logic to docs and always initialize 3rd
argument of such builtins.

Closes #136292

(cherry picked from commit 060f955)

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Pavel Skripkin (pskrgag)

Changes

According to
https://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins, result of builtin_*_overflow functions will be initialized even in case of overflow. Align analyzer logic to docs and always initialize 3rd argument of such builtins.

Closes #136292

---

Full diff: https://github.com/llvm/llvm-project/pull/136589.diff

3 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp (+48-38)
• (modified) clang/test/Analysis/builtin_overflow.c (+3-3)
• (modified) clang/test/Analysis/builtin_overflow_notes.c (+7-3)

diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index cfdd3c9faa360..bcc4ca77f5887 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -97,10 +97,14 @@ class BuiltinFunctionChecker : public Checker<eval::Call> {
   void handleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
                              BinaryOperator::Opcode Op,
                              QualType ResultType) const;
-  const NoteTag *createBuiltinNoOverflowNoteTag(CheckerContext &C,
-                                                bool BothFeasible, SVal Arg1,
-                                                SVal Arg2, SVal Result) const;
-  const NoteTag *createBuiltinOverflowNoteTag(CheckerContext &C) const;
+  const NoteTag *createBuiltinOverflowNoteTag(CheckerContext &C,
+                                              bool BothFeasible, SVal Arg1,
+                                              SVal Arg2, SVal Result) const;
+  ProgramStateRef initStateAftetBuiltinOverflow(CheckerContext &C,
+                                                ProgramStateRef State,
+                                                const CallEvent &Call,
+                                                SVal RetCal,
+                                                bool IsOverflow) const;
   std::pair<bool, bool> checkOverflow(CheckerContext &C, SVal RetVal,
                                       QualType Res) const;
 
@@ -122,30 +126,24 @@ class BuiltinFunctionChecker : public Checker<eval::Call> {
 
 } // namespace
 
-const NoteTag *BuiltinFunctionChecker::createBuiltinNoOverflowNoteTag(
-    CheckerContext &C, bool BothFeasible, SVal Arg1, SVal Arg2,
-    SVal Result) const {
-  return C.getNoteTag([Result, Arg1, Arg2, BothFeasible](
-                          PathSensitiveBugReport &BR, llvm::raw_ostream &OS) {
+const NoteTag *BuiltinFunctionChecker::createBuiltinOverflowNoteTag(
+    CheckerContext &C, bool overflow, SVal Arg1, SVal Arg2, SVal Result) const {
+  return C.getNoteTag([Result, Arg1, Arg2, overflow](PathSensitiveBugReport &BR,
+                                                     llvm::raw_ostream &OS) {
     if (!BR.isInteresting(Result))
       return;
 
-    // Propagate interestingness to input argumets if result is interesting.
+    // Propagate interestingness to input arguments if result is interesting.
     BR.markInteresting(Arg1);
     BR.markInteresting(Arg2);
 
-    if (BothFeasible)
+    if (overflow)
+      OS << "Assuming overflow";
+    else
       OS << "Assuming no overflow";
   });
 }
 
-const NoteTag *
-BuiltinFunctionChecker::createBuiltinOverflowNoteTag(CheckerContext &C) const {
-  return C.getNoteTag([](PathSensitiveBugReport &BR,
-                         llvm::raw_ostream &OS) { OS << "Assuming overflow"; },
-                      /*isPrunable=*/true);
-}
-
 std::pair<bool, bool>
 BuiltinFunctionChecker::checkOverflow(CheckerContext &C, SVal RetVal,
                                       QualType Res) const {
@@ -175,6 +173,29 @@ BuiltinFunctionChecker::checkOverflow(CheckerContext &C, SVal RetVal,
   return {MayOverflow || MayUnderflow, MayNotOverflow && MayNotUnderflow};
 }
 
+ProgramStateRef BuiltinFunctionChecker::initStateAftetBuiltinOverflow(
+    CheckerContext &C, ProgramStateRef State, const CallEvent &Call,
+    SVal RetVal, bool IsOverflow) const {
+  SValBuilder &SVB = C.getSValBuilder();
+  SVal Arg1 = Call.getArgSVal(0);
+  SVal Arg2 = Call.getArgSVal(1);
+  auto BoolTy = C.getASTContext().BoolTy;
+
+  ProgramStateRef NewState =
+      State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
+                      SVB.makeTruthVal(IsOverflow, BoolTy));
+
+  if (auto L = Call.getArgSVal(2).getAs<Loc>()) {
+    NewState = NewState->bindLoc(*L, RetVal, C.getLocationContext());
+
+    // Propagate taint if any of the arguments were tainted
+    if (isTainted(State, Arg1) || isTainted(State, Arg2))
+      NewState = addTaint(NewState, *L);
+  }
+
+  return NewState;
+}
+
 void BuiltinFunctionChecker::handleOverflowBuiltin(const CallEvent &Call,
                                                    CheckerContext &C,
                                                    BinaryOperator::Opcode Op,
@@ -184,8 +205,6 @@ void BuiltinFunctionChecker::handleOverflowBuiltin(const CallEvent &Call,
 
   ProgramStateRef State = C.getState();
   SValBuilder &SVB = C.getSValBuilder();
-  const Expr *CE = Call.getOriginExpr();
-  auto BoolTy = C.getASTContext().BoolTy;
 
   SVal Arg1 = Call.getArgSVal(0);
   SVal Arg2 = Call.getArgSVal(1);
@@ -195,29 +214,20 @@ void BuiltinFunctionChecker::handleOverflowBuiltin(const CallEvent &Call,
   SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
 
   auto [Overflow, NotOverflow] = checkOverflow(C, RetValMax, ResultType);
-  if (NotOverflow) {
-    ProgramStateRef StateNoOverflow = State->BindExpr(
-        CE, C.getLocationContext(), SVB.makeTruthVal(false, BoolTy));
-
-    if (auto L = Call.getArgSVal(2).getAs<Loc>()) {
-      StateNoOverflow =
-          StateNoOverflow->bindLoc(*L, RetVal, C.getLocationContext());
 
-      // Propagate taint if any of the argumets were tainted
-      if (isTainted(State, Arg1) || isTainted(State, Arg2))
-        StateNoOverflow = addTaint(StateNoOverflow, *L);
-    }
+  if (NotOverflow) {
+    auto NewState =
+        initStateAftetBuiltinOverflow(C, State, Call, RetVal, false);
 
-    C.addTransition(
-        StateNoOverflow,
-        createBuiltinNoOverflowNoteTag(
-            C, /*BothFeasible=*/NotOverflow && Overflow, Arg1, Arg2, RetVal));
+    C.addTransition(NewState, createBuiltinOverflowNoteTag(
+                                  C, /*overflow=*/false, Arg1, Arg2, RetVal));
   }
 
   if (Overflow) {
-    C.addTransition(State->BindExpr(CE, C.getLocationContext(),
-                                    SVB.makeTruthVal(true, BoolTy)),
-                    createBuiltinOverflowNoteTag(C));
+    auto NewState = initStateAftetBuiltinOverflow(C, State, Call, RetVal, true);
+
+    C.addTransition(NewState, createBuiltinOverflowNoteTag(C, /*overflow=*/true,
+                                                           Arg1, Arg2, RetVal));
   }
 }
 
diff --git a/clang/test/Analysis/builtin_overflow.c b/clang/test/Analysis/builtin_overflow.c
index 9d98ce7a1af45..d290333071dc9 100644
--- a/clang/test/Analysis/builtin_overflow.c
+++ b/clang/test/Analysis/builtin_overflow.c
@@ -26,7 +26,7 @@ void test_add_overflow(void)
    int res;
 
    if (__builtin_add_overflow(__INT_MAX__, 1, &res)) {
-     clang_analyzer_dump_int(res); //expected-warning{{1st function call argument is an uninitialized value}}
+     clang_analyzer_dump_int(res); //expected-warning{{-2147483648 S32b}}
      return;
    }
 
@@ -38,7 +38,7 @@ void test_add_underoverflow(void)
    int res;
 
    if (__builtin_add_overflow(__INT_MIN__, -1, &res)) {
-     clang_analyzer_dump_int(res); //expected-warning{{1st function call argument is an uninitialized value}}
+     clang_analyzer_dump_int(res); //expected-warning{{2147483647 S32b}}
      return;
    }
 
@@ -160,7 +160,7 @@ void test_bool_assign(void)
 {
     int res;
 
-    // Reproduce issue from GH#111147. __builtin_*_overflow funcions
+    // Reproduce issue from GH#111147. __builtin_*_overflow functions
     // should return _Bool, but not int.
     _Bool ret = __builtin_mul_overflow(10, 20, &res); // no crash
 }
diff --git a/clang/test/Analysis/builtin_overflow_notes.c b/clang/test/Analysis/builtin_overflow_notes.c
index 20f333a4a6cca..94c79b5ed334a 100644
--- a/clang/test/Analysis/builtin_overflow_notes.c
+++ b/clang/test/Analysis/builtin_overflow_notes.c
@@ -19,12 +19,16 @@ void test_no_overflow_note(int a, int b)
 
 void test_overflow_note(int a, int b)
 {
-   int res; // expected-note{{'res' declared without an initial value}}
+   int res;
 
    if (__builtin_add_overflow(a, b, &res)) { // expected-note {{Assuming overflow}}
                                              // expected-note@-1 {{Taking true branch}}
-     int var = res; // expected-warning{{Assigned value is garbage or undefined}}
-                    // expected-note@-1 {{Assigned value is garbage or undefined}}
+     if (res) { // expected-note {{Assuming 'res' is not equal to 0}}
+                // expected-note@-1 {{Taking true branch}}
+        int *ptr = 0; // expected-note {{'ptr' initialized to a null pointer value}}
+        int var = *(int *) ptr; //expected-warning {{Dereference of null pointer}}
+                                //expected-note@-1 {{Dereference of null pointer}}
+     }
      return;
    }
 }

[pskrgag]

Hm, I guess I've done smth wrong, since merge button is not available? Sorry, I am doing llvm backports for the first time

[steakhal]

Please checkout the release branch, reset hard to reset to that one. Then issue the "git cherry-pick -x HASH" command and force push to your branch.
This should get us a nice cherry picked from comment in the message.

I think only the release manager can accept merges, so its not your fault.
Maintainers like me are expected to review and accept backports to take responsibility.

[steakhal]

Make sure after the force push you sync zhe PR summary with the commit message.

[pskrgag]

Then issue the "git cherry-pick -x HASH" command and force push to your branch.

I've done that locally (+ fixed merge conflicts) and commit message is the same as in original patch. I've only changed the PR title

[steakhal]

I've updated the commit description to have the desired cherry pick marker line, referring to the commit hash picked.
It's ready to go now.

[steakhal]

Since I did the update to this PR the way I wanted, I invite another code owner to approve. /cc @Xazax-hun