NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:201
+ std::tie(StateRetNotNull, StateRetNull) =
+ CM.assumeDual(StateStreamNull, RetVal);
+ if (StateRetNull) {
----------------
baloghadamsoftware wrote:
> 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.
The final code looks correct to me, thanks!~
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
