[analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' #100085

steakhal · 2024-07-23T09:10:43Z

'fopen' should return a new FILE handle, thus we should assume it can't alias with commonly used FILE handles, such as with 'stdin', 'stdout' or 'stderr'.

This problem appears in code that handles either some input/output file with stdin or stdout, as the business logic is basically the same no matter the stream being used.
However, one would should only close the stream if it was opened via 'fopen'. Consequently, such code usually has a condition like if (f && f != stdout) to guard the fclose() call.

This patch brings this assumption, thus eliminates FPs for not taking the guarded branch.

CPP-5306

…err}' 'fopen' should return a new FILE handle, thus we should assume it can't alias with commonly used FILE handles, such as with 'stdin', 'stdout' or 'stderr'. This problem appears in code that handles either some input/output file with stdin or stdout, as the business logic is basically the same no matter the stream being used. However, one would should only close the stream if it was opened via 'fopen'. Consequently, such code usually has a condition like `if (f && f != stdout)` to guard the `fclose()` call. This patch brings this assumption, thus eliminates FPs for not taking the guarded branch. CPP-5306

llvmbot · 2024-07-23T09:11:07Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balazs Benics (steakhal)

Changes

'fopen' should return a new FILE handle, thus we should assume it can't alias with commonly used FILE handles, such as with 'stdin', 'stdout' or 'stderr'.

This problem appears in code that handles either some input/output file with stdin or stdout, as the business logic is basically the same no matter the stream being used.
However, one would should only close the stream if it was opened via 'fopen'. Consequently, such code usually has a condition like if (f && f != stdout) to guard the fclose() call.

This patch brings this assumption, thus eliminates FPs for not taking the guarded branch.

CPP-5306

Full diff: https://github.com/llvm/llvm-project/pull/100085.diff

2 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+71-1)
(modified) clang/test/Analysis/stream.c (+11)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index e8d538388e56c..53770532609d5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -254,7 +254,8 @@ inline void assertStreamStateOpened(const StreamState *SS) {
 }
 
 class StreamChecker : public Checker<check::PreCall, eval::Call,
-                                     check::DeadSymbols, check::PointerEscape> {
+                                     check::DeadSymbols, check::PointerEscape,
+                                     check::ASTDecl<TranslationUnitDecl>> {
   BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
   BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};
   BugType BT_UseAfterOpenFailed{this, "Invalid stream",
@@ -276,11 +277,21 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
                                      const CallEvent *Call,
                                      PointerEscapeKind Kind) const;
 
+  /// Finds the declarations of 'FILE *stdin, *stdout, *stderr'.
+  void checkASTDecl(const TranslationUnitDecl *TU, AnalysisManager &,
+                    BugReporter &) const;
+
   const BugType *getBT_StreamEof() const { return &BT_StreamEof; }
   const BugType *getBT_IndeterminatePosition() const {
     return &BT_IndeterminatePosition;
   }
 
+  /// Assumes that the result of 'fopen' can't alias with the pointee of
+  /// 'stdin', 'stdout' or 'stderr'.
+  ProgramStateRef assumeNoAliasingWithStdStreams(ProgramStateRef State,
+                                                 DefinedSVal RetVal,
+                                                 CheckerContext &C) const;
+
   const NoteTag *constructSetEofNoteTag(CheckerContext &C,
                                         SymbolRef StreamSym) const {
     return C.getNoteTag([this, StreamSym](PathSensitiveBugReport &BR) {
@@ -451,6 +462,10 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   /// The built-in va_list type is platform-specific
   mutable QualType VaListType;
 
+  mutable const VarDecl *StdinDecl = nullptr;
+  mutable const VarDecl *StdoutDecl = nullptr;
+  mutable const VarDecl *StderrDecl = nullptr;
+
   void evalFopen(const FnDescription *Desc, const CallEvent &Call,
                  CheckerContext &C) const;
 
@@ -887,6 +902,30 @@ bool StreamChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
   return C.isDifferent();
 }
 
+ProgramStateRef StreamChecker::assumeNoAliasingWithStdStreams(
+    ProgramStateRef State, DefinedSVal RetVal, CheckerContext &C) const {
+  auto assumeRetNE = [&C, RetVal](ProgramStateRef State,
+                                  const VarDecl *Var) -> ProgramStateRef {
+    if (!Var)
+      return State;
+    const auto *LCtx = C.getLocationContext();
+    auto &StoreMgr = C.getStoreManager();
+    auto &SVB = C.getSValBuilder();
+    SVal VarValue = State->getSVal(StoreMgr.getLValueVar(Var, LCtx));
+    auto NoAliasState =
+        SVB.evalBinOp(State, BO_NE, RetVal, VarValue, SVB.getConditionType())
+            .castAs<DefinedOrUnknownSVal>();
+    return State->assume(NoAliasState, true);
+  };
+
+  assert(State);
+  State = assumeRetNE(State, StdinDecl);
+  State = assumeRetNE(State, StdoutDecl);
+  State = assumeRetNE(State, StderrDecl);
+  assert(State);
+  return State;
+}
+
 void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
                               CheckerContext &C) const {
   ProgramStateRef State = C.getState();
@@ -899,6 +938,7 @@ 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.
@@ -2017,6 +2057,36 @@ ProgramStateRef StreamChecker::checkPointerEscape(
   return State;
 }
 
+static const VarDecl *
+getGlobalStreamPointerByName(const TranslationUnitDecl *TU, StringRef VarName) {
+  ASTContext &Ctx = TU->getASTContext();
+  const auto &SM = Ctx.getSourceManager();
+  const QualType FileTy = Ctx.getFILEType();
+
+  if (FileTy.isNull())
+    return nullptr;
+
+  const QualType FilePtrTy = Ctx.getPointerType(FileTy).getCanonicalType();
+
+  auto LookupRes = TU->lookup(&Ctx.Idents.get(VarName));
+  for (const Decl *D : LookupRes) {
+    if (auto *VD = dyn_cast_or_null<VarDecl>(D)) {
+      if (SM.isInSystemHeader(VD->getLocation()) && VD->hasExternalStorage() &&
+          VD->getType().getCanonicalType() == FilePtrTy) {
+        return VD;
+      }
+    }
+  }
+  return nullptr;
+}
+
+void StreamChecker::checkASTDecl(const TranslationUnitDecl *TU,
+                                 AnalysisManager &, BugReporter &) const {
+  StdinDecl = getGlobalStreamPointerByName(TU, "stdin");
+  StdoutDecl = getGlobalStreamPointerByName(TU, "stdout");
+  StderrDecl = getGlobalStreamPointerByName(TU, "stderr");
+}
+
 //===----------------------------------------------------------------------===//
 // Checker registration.
 //===----------------------------------------------------------------------===//
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index c924cbd36f759..c518820df6ed6 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -498,3 +498,14 @@ void gh_93408_regression_ZeroSized(struct ZeroSized *buffer) {
   fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash
   fclose(f);
 }
+
+extern FILE *stdout_like_ptr;
+void no_aliasing(void) {
+  FILE *f = fopen("file", "r");
+  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) {
+    fclose(f);
+  }
+} // no-leak: 'fclose()' is always called because 'f' cannot be 'stdout'.

steakhal · 2024-07-23T09:11:07Z

I didn't mention this in the release notes as I don't think it's that impactful.

NagyDonat

LGTM, this is a nice improvement. I vaguely recall that a few months ago somebody else on our team had trouble with false positives similar false positives, so it's good to see that these will be fixed.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

balazske

I was not fast enough but have some comments. I had plans already about recognizing the std streams but the case looked more difficult. These streams are just global variables that can be changed (assigned, opened, closed) by the program. Theoretically it is not possible to know anything about these after invalidation. This patch works only if the standard streams are not manipulated by the program (it is rare but not impossible). I had the idea that a checker option should be added to model these streams (or not). Then some more things can be improved.

balazske · 2024-07-23T13:18:49Z

clang/test/Analysis/stream.c

@@ -498,3 +498,15 @@ void gh_93408_regression_ZeroSized(struct ZeroSized *buffer) {
  fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash
  fclose(f);
 }
+
+extern FILE *stdout_like_ptr;
+void no_aliasing(void) {


This test could have a more meaningful name.

I think nobody will ever come back to this test. I don't think I'll elaborate the name. The content should be enough already.

balazske · 2024-07-23T13:20:35Z

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

+  return nullptr;
+}
+
+void StreamChecker::checkASTDecl(const TranslationUnitDecl *TU,


The checker has already initialization functions in checkPreCall (initMacroValues, initVaListType). These can now be moved to here.

Yea, this makes sense to harmonize. I'll try to come back to this one later, or write a good first issue for cleaning this up.

steakhal · 2024-07-23T13:37:37Z

[...] This patch works only if the standard streams are not manipulated by the program (it is rare but not impossible). I had the idea that a checker option should be added to model these streams (or not). Then some more things can be improved.

Makes sense, but I think a checker option is overkill until we find a complaint for this.
To me, assuming no-aliasing is the right assumption here.

Thanks for the comments :)

AaronBallman · 2024-07-23T13:42:04Z

Given that stdout and friends are regular variables in some libc implementations (https://www.gnu.org/software/libc/manual/html_node/Standard-Streams.html), the result can alias, can't it? e.g.,

fclose(stdout);
FILE *fp;
fp = stdout = fopen("blah", "w");

steakhal · 2024-07-23T15:44:31Z

@AaronBallman You are technically right. But it makes more sense to assume they don't alias. Luckily, in CSA, we don't need to be perfect, and covering the generic scenario usually leads to better user experience.

That said, I'll add the example you suggested.

AaronBallman · 2024-07-23T15:50:22Z

@AaronBallman You are technically right. But it makes more sense to assume they don't alias. Luckily, in CSA, we don't need to be perfect, and covering the generic scenario usually leads to better user experience.

That said, I'll add the example you suggested.

Okay, that seems reasonable enough to me, thank you!

balazske · 2024-07-24T08:05:48Z

Documentation of the checker could be updated with this new behavior.

…err}' (#100085) 'fopen' should return a new FILE handle, thus we should assume it can't alias with commonly used FILE handles, such as with 'stdin', 'stdout' or 'stderr'. This problem appears in code that handles either some input/output file with stdin or stdout, as the business logic is basically the same no matter the stream being used. However, one would should only close the stream if it was opened via 'fopen'. Consequently, such code usually has a condition like `if (f && f != stdout)` to guard the `fclose()` call. This patch brings this assumption, thus eliminates FPs for not taking the guarded branch. CPP-5306

steakhal added the clang:static analyzer label Jul 23, 2024

steakhal requested review from Xazax-hun, balazske and NagyDonat July 23, 2024 09:10

llvmbot added the clang Clang issues not falling into any other category label Jul 23, 2024

Also check aliasing against stdin

a753f0c

NagyDonat approved these changes Jul 23, 2024

View reviewed changes

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp Show resolved Hide resolved

steakhal merged commit b60fec2 into llvm:main Jul 23, 2024
7 checks passed

steakhal deleted the bb/cpp-5306-fopen-no-alias branch July 23, 2024 11:23

balazske reviewed Jul 23, 2024

View reviewed changes

vabridgers mentioned this pull request Jul 28, 2024

[analyzer] Avoid crashes in the stream checker #100901

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' #100085

[analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' #100085

Uh oh!

steakhal commented Jul 23, 2024

Uh oh!

llvmbot commented Jul 23, 2024 •

edited

Loading

Uh oh!

steakhal commented Jul 23, 2024

Uh oh!

NagyDonat left a comment

Uh oh!

Uh oh!

Uh oh!

balazske left a comment

Uh oh!

balazske Jul 23, 2024

Uh oh!

steakhal Jul 23, 2024

Uh oh!

balazske Jul 23, 2024

Uh oh!

steakhal Jul 23, 2024

Uh oh!

steakhal commented Jul 23, 2024

Uh oh!

AaronBallman commented Jul 23, 2024

Uh oh!

steakhal commented Jul 23, 2024

Uh oh!

AaronBallman commented Jul 23, 2024

Uh oh!

balazske commented Jul 24, 2024

Uh oh!

Uh oh!

[analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' #100085

[analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' #100085

Uh oh!

Conversation

steakhal commented Jul 23, 2024

Uh oh!

llvmbot commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steakhal commented Jul 23, 2024

Uh oh!

NagyDonat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

balazske left a comment

Choose a reason for hiding this comment

Uh oh!

balazske Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

steakhal Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

balazske Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

steakhal Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

steakhal commented Jul 23, 2024

Uh oh!

AaronBallman commented Jul 23, 2024

Uh oh!

steakhal commented Jul 23, 2024

Uh oh!

AaronBallman commented Jul 23, 2024

Uh oh!

balazske commented Jul 24, 2024

Uh oh!

Uh oh!

llvmbot commented Jul 23, 2024 •

edited

Loading