llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balazs Benics (steakhal) <details> <summary>Changes</summary> Actually, on the failure branch of `fopen`, the resulting pointer could alias with `stdout` iff `stdout` is already known to be null. We crashed in this case as the implementation assumed that the state-split for creating the success and failure branches both should be viable; thus dereferenced both of those states - leading to the crash. To fix this, let's just only add this no-alias property for the success path, and that's it :) Fixes #<!-- -->100901 --- Full diff: https://github.com/llvm/llvm-project/pull/100990.diff 3 Files Affected: - (modified) clang/docs/analyzer/checkers.rst (+5-8) - (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+10-18) - (modified) clang/test/Analysis/stream.c (+31-11) ``````````diff diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 76a9aae170893..452948aa2cb00 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -1703,7 +1703,11 @@ are detected: * Invalid 3rd ("``whence``") argument to ``fseek``. The stream operations are by this checker usually split into two cases, a success -and a failure case. However, in the case of write operations (like ``fwrite``, +and a failure case. +On the success case it also assumes that the current value of ``stdout``, +``stderr``, or ``stdin`` can't be equal to the file pointer returned by ``fopen``. + +In the case of write operations (like ``fwrite``, ``fprintf`` and even ``fsetpos``) this behavior could produce a large amount of unwanted reports on projects that don't have error checks around the write operations, so by default the checker assumes that write operations always succeed. @@ -1766,13 +1770,6 @@ are assumed to succeed.) fclose(p); } -**Limitations** - -The checker does not track the correspondence between integer file descriptors -and ``FILE *`` pointers. Operations on standard streams like ``stdin`` are not -treated specially and are therefore often not recognized (because these streams -are usually not opened explicitly by the program, and are global variables). - .. _osx-checkers: osx diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 53770532609d5..4454f30630e27 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -615,30 +615,22 @@ class StreamChecker : public Checker<check::PreCall, eval::Call, }); } - void initMacroValues(CheckerContext &C) const { + void initMacroValues(const Preprocessor &PP) const { if (EofVal) return; - if (const std::optional<int> OptInt = - tryExpandAsInteger("EOF", C.getPreprocessor())) + if (const std::optional<int> OptInt = tryExpandAsInteger("EOF", PP)) EofVal = *OptInt; else EofVal = -1; - if (const std::optional<int> OptInt = - tryExpandAsInteger("SEEK_SET", C.getPreprocessor())) + if (const std::optional<int> OptInt = tryExpandAsInteger("SEEK_SET", PP)) SeekSetVal = *OptInt; - if (const std::optional<int> OptInt = - tryExpandAsInteger("SEEK_END", C.getPreprocessor())) + if (const std::optional<int> OptInt = tryExpandAsInteger("SEEK_END", PP)) SeekEndVal = *OptInt; - if (const std::optional<int> OptInt = - tryExpandAsInteger("SEEK_CUR", C.getPreprocessor())) + if (const std::optional<int> OptInt = tryExpandAsInteger("SEEK_CUR", PP)) SeekCurVal = *OptInt; } - void initVaListType(CheckerContext &C) const { - VaListType = C.getASTContext().getBuiltinVaListType().getCanonicalType(); - } - /// Searches for the ExplodedNode where the file descriptor was acquired for /// StreamSym. static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N, @@ -880,9 +872,6 @@ static ProgramStateRef escapeArgs(ProgramStateRef State, CheckerContext &C, void StreamChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { - initMacroValues(C); - initVaListType(C); - const FnDescription *Desc = lookupFn(Call); if (!Desc || !Desc->PreFn) return; @@ -938,7 +927,6 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call, assert(RetSym && "RetVal must be a symbol here."); State = State->BindExpr(CE, C.getLocationContext(), RetVal); - State = assumeNoAliasingWithStdStreams(State, RetVal, C); // Bifurcate the state into two: one with a valid FILE* pointer, the other // with a NULL. @@ -951,6 +939,8 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call, StateNull = StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc)); + StateNotNull = assumeNoAliasingWithStdStreams(StateNotNull, RetVal, C); + C.addTransition(StateNotNull, constructLeakNoteTag(C, RetSym, "Stream opened here")); C.addTransition(StateNull); @@ -2081,10 +2071,12 @@ getGlobalStreamPointerByName(const TranslationUnitDecl *TU, StringRef VarName) { } void StreamChecker::checkASTDecl(const TranslationUnitDecl *TU, - AnalysisManager &, BugReporter &) const { + AnalysisManager &Mgr, BugReporter &) const { StdinDecl = getGlobalStreamPointerByName(TU, "stdin"); StdoutDecl = getGlobalStreamPointerByName(TU, "stdout"); StderrDecl = getGlobalStreamPointerByName(TU, "stderr"); + VaListType = TU->getASTContext().getBuiltinVaListType().getCanonicalType(); + initMacroValues(Mgr.getPreprocessor()); } //===----------------------------------------------------------------------===// diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c index b3a47ce4153d3..b9a5b1ba8cd49 100644 --- a/clang/test/Analysis/stream.c +++ b/clang/test/Analysis/stream.c @@ -1,11 +1,11 @@ // RUN: %clang_analyze_cc1 -triple=x86_64-pc-linux-gnu -analyzer-checker=core,unix.Stream,debug.ExprInspection \ -// RUN: -analyzer-config unix.Stream:Pedantic=true -verify %s +// RUN: -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s // RUN: %clang_analyze_cc1 -triple=armv8-none-linux-eabi -analyzer-checker=core,unix.Stream,debug.ExprInspection \ -// RUN: -analyzer-config unix.Stream:Pedantic=true -verify %s +// RUN: -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s // RUN: %clang_analyze_cc1 -triple=aarch64-linux-gnu -analyzer-checker=core,unix.Stream,debug.ExprInspection \ -// RUN: -analyzer-config unix.Stream:Pedantic=true -verify %s +// RUN: -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s // RUN: %clang_analyze_cc1 -triple=hexagon -analyzer-checker=core,unix.Stream,debug.ExprInspection \ -// RUN: -analyzer-config unix.Stream:Pedantic=true -verify %s +// RUN: -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s #include "Inputs/system-header-simulator.h" #include "Inputs/system-header-simulator-for-malloc.h" @@ -499,14 +499,34 @@ void gh_93408_regression_ZeroSized(struct ZeroSized *buffer) { fclose(f); } -extern FILE *stdout_like_ptr; -void no_aliasing(void) { +extern FILE *non_standard_stream_ptr; +void test_fopen_does_not_alias_with_standard_streams(void) { FILE *f = fopen("file", "r"); - clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}} no-TRUE - clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}} no-TRUE - clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}} no-TRUE - clang_analyzer_eval(f == stdout_like_ptr); // expected-warning {{FALSE}} expected-warning {{TRUE}} - if (f && f != stdout) { + if (!f) return; + clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}} no-TRUE + clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}} no-TRUE + clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}} no-TRUE + clang_analyzer_eval(f == non_standard_stream_ptr); // expected-warning {{UNKNOWN}} + if (f != stdout) { fclose(f); } } // no-leak: 'fclose()' is always called because 'f' cannot be 'stdout'. + +void reopen_std_stream(void) { + FILE *oldStdout = stdout; + fclose(stdout); + FILE *fp = fopen("blah", "w"); + if (!fp) return; + + stdout = fp; // Let's make them alias. + clang_analyzer_eval(fp == oldStdout); // expected-warning {{UNKNOWN}} + clang_analyzer_eval(fp == stdout); // expected-warning {{TRUE}} no-FALSE + clang_analyzer_eval(oldStdout == stdout); // expected-warning {{UNKNOWN}} +} + +void only_success_path_does_not_alias_with_stdout(void) { + if (stdout) return; + FILE *f = fopen("/tmp/foof", "r"); // no-crash + if (!f) return; + fclose(f); +} `````````` </details> https://github.com/llvm/llvm-project/pull/100990 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
