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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
