[analyzer] Fix StdLibraryFunctionsChecker crash on surprising sink node

[NagyDonat]

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.

[llvmbot]

@llvm/pr-subscribers-clang

Changes

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.

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

1 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp (+8-3)

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

[llvmbot]

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

Changes

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.

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

1 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp (+8-3)

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

[steakhal]

I don't think it deserves a fragile test case.
I think the fix looks good.
I also agree with your other statements on the addTransition subject.