Skip to content

Commit

Permalink
[analyzer] Fix StdLibraryFunctionsChecker crash on surprising sink no…
Browse files Browse the repository at this point in the history
…de (#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.
  • Loading branch information
NagyDonat committed Sep 14, 2023
1 parent e873280 commit 0b2778d
Showing 1 changed file with 8 additions and 3 deletions.
11 changes: 8 additions & 3 deletions clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -1427,7 +1427,12 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
});
Pred = C.addTransition(NewState, Pred, Tag);
}
if (!Pred)

// Pred may be:
// - a nullpointer, if we reach an already existing node (theoretically);
// - a sink, when NewState is posteriorly overconstrained.
// In these situations we cannot add the errno note tag.
if (!Pred || Pred->isSink())
continue;
}

Expand Down

0 comments on commit 0b2778d

Please sign in to comment.