[clang] [analyzer] Fix StdLibraryFunctionsChecker crash on surprising sink node (PR #66109)

[via cfe-commits]

https://github.com/DonatNagyE created https://github.com/llvm/llvm-project/pull/66109:

Recent changes in StdLibraryFunctionsChecker introduced a situation where the checker sequentially performed two state transitions to add two separate note tags.

In the unlikely case when the updated state (the variable `NewState`) was posteriorly overconstrained, the engine marked the node after the first state transition as a sink to stop the "natural" graph exploration after that point.

However, in this particular case the checker tried to directly add a second node, and this triggered an assertion in the `addPredecessor()` method of `ExplodedNode`.

This commit introduces an explicit `isSink()` check to avoid this crash. To avoid similar bugs in the future, perhaps it would be possible to tweak `addTransition()` and ensure that it returns `nullptr` when it would return a sink node (to unify the two possible error conditions).

This crash was observed in an analysis of the curl project (in a very long and complex function), and there I validated that this is the root cause, but I don't have a self-contained testcase that can trigger the creation of a PosteriorlyOverconstrained node in this situation.

>From 48deb9bab94e0fbf9d9ae90d51c733841fa1dbb1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Tue, 12 Sep 2023 18:07:34 +0200
Subject: [PATCH] [analyzer] Fix StdLibraryFunctionsChecker crash on surprising
 sink node

Recent changes in StdLibraryFunctionsChecker introduced a situation
where the checker sequentially performed two state transitions to add
two separate note tags.

In the unlikely case when the updated state (the variable `NewState`)
was posteriorly overconstrained, the engine marked the node after the
first state transition as a sink to stop the "natural" graph exploration
after that point.

However, in this particular case the checker tried to directly add a
second node, and this triggered an assertion in the `addPredecessor()`
method of `ExplodedNode`.

This commit introduces an explicit `isSink()` check to avoid this crash.
To avoid similar bugs in the future, perhaps it would be possible to
tweak `addTransition()` and ensure that it returns `nullptr` when it
would return a sink node (to unify the two possible error conditions).

This crash was observed in an analysis of the curl project (in a very
long and complex function), and there I validated that this is the root
cause, but I don't have a self-contained testcase that can trigger the
creation of a PosteriorlyOverconstrained node in this situation.
---
 .../Checkers/StdLibraryFunctionsChecker.cpp           | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index f5f6e3a91237686..d3a41dd9aff2891 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -1387,8 +1387,8 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
     if (!NewState)
       continue;
 
-    // It is possible that NewState == State is true.
-    // It can occur if another checker has applied the state before us.
+    // Here it's possible that NewState == State, e.g. when other checkers
+    // already applied the same constraints (or stricter ones).
     // Still add these note tags, the other checker should add only its
     // specialized note tags. These general note tags are handled always by
     // StdLibraryFunctionsChecker.
@@ -1427,8 +1427,13 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
             });
         Pred = C.addTransition(NewState, Pred, Tag);
       }
-      if (!Pred)
+      if (!Pred || Pred->isSink()) {
+        // Pred may be:
+        //  - a nullpointer, when the generated node is not new;
+        //  - a sink, when NewState is posteriorly overconstrained.
+        // In these situations we cannot add the errno note tag.
         continue;
+      }
     }
 
     // If we can get a note tag for the errno change, add this additionally to