Szelethus added a comment.
To summarize, we're moving every stream check (illegal call to a stream
management function with a null stream, or a closed stream, etc) to `preCall`,
and leave only the modeling portions in `evalCall`. Makes sense!
You did an amazing job here! I like the new infrastructure and everything.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:76-92
+ {{"fopen"}, {{}, &StreamChecker::evalFopen, ArgNone}},
+ {{"freopen", 3},
+ {&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2}},
+ {{"tmpfile"}, {{}, &StreamChecker::evalFopen, ArgNone}},
+ {{"fclose", 1},
+ {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}},
+ {{"fread", 4}, {&StreamChecker::preDefault, {}, 3}},
----------------
Hmm, all of these braces are kinda hard to navigate, but its not too bad. Could
you check whether changing them to `nullptr` improves readability? I'll leave
it to your judgement.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:109-114
+ ProgramStateRef ensureStreamNonNull(SVal StreamVal, CheckerContext &C,
+ ProgramStateRef State) const;
+ ProgramStateRef ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
+ ProgramStateRef State) const;
+ ProgramStateRef ensureStreamOpened(SVal StreamVal, CheckerContext &C,
+ ProgramStateRef State) const;
----------------
Could we add/move some comments for these? :)
```
Checks whether the stream is non-null. If it is null, an fatal bug report is
emitted and the return value is nullptr. Otherwise in the returned state
StreamVal is non-null.
```
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:161
+ ProgramStateRef State = C.getState();
+ SVal StreamVal = Call.getArgSVal(Desc->StreamArgNo);
+ State = ensureStreamNonNull(StreamVal, C, State);
----------------
This would assert in `Expr::getArg` if `Desc->StreamArgNo == ArgNone`, but that
shouldn't ever happen in this function, correct? Could we add an assert here as
well as a poor man's documentation?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:192-194
+ // Do not allow NULL as passed stream pointer.
+ // This is not specified in the man page but may crash on some system.
+ // But allow a closed stream, is this correct?
----------------
According to my research, yes, closed streams are okay. Also, the rest of the
comment can be removed as well -- it is for sure not okay to pass `NULL` to
`freopen`, even if it doesn't crash on some systems.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:281-284
+ // Close the File Descriptor.
+ // Regardless if the close fails or not, stream becomes "closed"
+ // and can not be used any more.
+ State = State->set<StreamMap>(Sym, StreamState::getClosed());
----------------
Ah, we're moving the closing of the stream from `checkDoubleClose` to here.
Makes much more sense!
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:341-345
+// Check:
+// Using a stream pointer after 'fclose' causes undefined behavior
+// according to cppreference.com .
+// Using a stream that has failed to open is likely to cause problems, but not
+// explicitly mentioned in documentation.
----------------
Could we move this to the declaration and start it with `///`?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:349-355
+ SymbolRef Sym = StreamVal.getAsSymbol();
if (!Sym)
- return State;
+ return nullptr;
const StreamState *SS = State->get<StreamMap>(Sym);
-
- // If the file stream is not tracked, return.
if (!SS)
+ return nullptr;
----------------
Previously, we only returned a `nullptr` after generating a fatal error node.
Why do we need to change this?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:363-364
+ new BuiltinBug(this, "Stream used after close",
+ "File descriptor is used after it was closed. "
+ "Cause undefined behaviour."));
C.emitReport(std::make_unique<PathSensitiveBugReport>(
----------------
How about
"Closing an already closed file descriptor."
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:381-382
+ new BuiltinBug(this, "Stream used after failed open",
+ "(Re-)Opening the file descriptor has failed. "
+ "Using it can cause undefined behaviour."));
+ C.emitReport(std::make_unique<PathSensitiveBugReport>(
----------------
How about
"Using a file descriptor after (re-)opening it failed."
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75163/new/
https://reviews.llvm.org/D75163
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
