[PATCH] D69948: [Checkers] Added support for freopen to StreamChecker.

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:201
+    std::tie(StateRetNotNull, StateRetNull) =
+        CM.assumeDual(StateStreamNull, RetVal);
+    if (StateRetNull) {
----------------
balazske wrote:
> NoQ wrote:
> > Mmm, wait, this doesn't look right to me. You cannot deduce from the presence of `freopen()` in the code that the argument may be null, so the split over the argument is not well-justified.
> > 
> > The right thing to do here will be to produce a "Schrödinger file descriptor" that's both `Opened` and `OpenFailed` until we observe the return value to be constrained to null or non-null later during analysis (cf. D32449), otherwise conservatively assume that the open succeeded when queried for the first time (because that's how non-paranoid code under analysis will behave).
> > 
> > Or you could drop the failed path immediately if the argument isn't known to be zero. That'll drop the coverage slightly but won't cause anything bad to happen.
> > 
> > 
> Where does this comment belong? I got this in the email:
> ```
> Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:201
> +    std::tie(StateRetNotNull, StateRetNull) =
> +        CM.assumeDual(StateStreamNull, RetVal);
> +    if (StateRetNull) {
> ```
> Is the problem with the null-check of argument of freopen? Why is this different than the other calls where `checkNullStream` is used? 
> Or is the problem with the case of NULL return value from `freopen` (in this case the argument was non-null, for the null case we assume program crash)? In this case we really do not know what happened with the file descriptor argument (but the value of it is not changed), so the code assumes now that is will be OpenFailed. This is not accurate always (it may remain open or become closed). We could have another state split here (for these cases)?
The argument is checked against `NULL`, becuase it must not be `NULL`. That is the "checker" part. The return value is split like in the existing code for modeling `fopen()`. I do not see anything wrong here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:206
+      StateRetNotNull->set<StreamMap>(StreamSym, StreamState::getOpened());
+  C.addTransition(StateRetNotNull);
+}
----------------
I think it would be better to rearrange these lines to exactly match `evalFopen()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69948/new/

https://reviews.llvm.org/D69948