[clang][analyzer] Add errno modeling to StreamChecker

[balazske]

Summary

Update the existing modeling of stream functions in StreamChecker
with 'errno' modeling. The not (yet) evaluated stream functions
are not updated.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

283–284

Have you considered using llvm::Optional<int> for this?

537

I believe, getDecl() might return null, e.g. for calling a function pointer.

clang/test/Analysis/stream-errno.c

12–13

So these two lines correspond to the following clang_analyzer_eval().
It took me a while to make sense of the two independent note streams at once.
Please consider splitting this and similar cases to have a single stream of notes.

[balazske]

StdLibraryFunctionsChecker should not overwrite errno constraints
that are set by StreamChecker.

[balazske]

• Use Optional for EofInitialized
• Split tests into two files with and without note check.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

537

In this case the function should be known (we know that fopen is the called function, if the name is known the Decl should be known too), I think it can not be null here.

[martong]

LGTM, thanks!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

465 ↗

(On Diff #454026)

Makes sense.

[balazske]

A new solution is in D135247. The new approach is that only StdLibraryFunctionsChecker adds the errno modeling part, together with other checks (that are applicable if StreamChecker is turned off) for pre and post conditions at the stream functions.

clang/docs/ReleaseNotes.rst

 --------------------
 
 libclang
 --------
 
 Static Analyzer
 ---------------
 
+- Checker ``alpha.unix.Stream`` is improved to be able to detect bugs related
+  to bad ``errno`` usage. The effects are visible if checker
+  ``alpha.unix.Errno`` is turned on additionally.
+
 .. _release-notes-ubsan:
 
 Undefined Behavior Sanitizer (UBSan)
 ------------------------------------
 
 Core Analysis Improvements
 ==========================
 

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

 //===-- StreamChecker.cpp -----------------------------------------*- C++ -*--//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines checkers that model and check stream handling functions.
 //
 //===----------------------------------------------------------------------===//
 
+#include "ErrnoModeling.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include <functional>
 
 using namespace clang;
 using namespace ento;
 using namespace std::placeholders;
CallDescriptionMap<FnDescription> FnTestDescriptions = {
         0}},
       {{"StreamTesterChecker_make_ferror_stream", 1},
        {nullptr,
         std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4,
                   ErrorFError),
         0}},
   };
 
+  mutable bool EofInitialized = false;
+  mutable int EofVal = -1;
+
   void evalFopen(const FnDescription *Desc, const CallEvent &Call,
                  CheckerContext &C) const;
 
   void preFreopen(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
   void evalFreopen(const FnDescription *Desc, const CallEvent &Call,
                    CheckerContext &C) const;
 
return C.getNoteTag([this, StreamSym](PathSensitiveBugReport &BR) {
         return "";
 
       BR.markNotInteresting(StreamSym);
 
       return "Assuming stream reaches end-of-file here";
     });
   }
 
+  /// Get a SVal to be used as new errno value at failure cases of stream
+  /// functions.
+  NonLoc getErrnoSVal(CheckerContext &C, const CallEvent &Call) const {
+    return C.getSValBuilder()
+        .conjureSymbolVal(this, Call.getOriginExpr(), C.getLocationContext(),
+                          C.getASTContext().IntTy, C.blockCount())
+        .castAs<NonLoc>();
+  }
+
+  void initEof(CheckerContext &C) const {
+    if (EofInitialized)
+      return;
+
+    if (const llvm::Optional<int> OptInt =
+            tryExpandAsInteger("EOF", C.getPreprocessor()))
+      EofVal = *OptInt;
+    EofInitialized = true;
+  }
+
   /// Searches for the ExplodedNode where the file descriptor was acquired for
   /// StreamSym.
   static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
                                                 SymbolRef StreamSym,
                                                 CheckerContext &C);
 };
 
 } // end anonymous namespace

 }
 
 //===----------------------------------------------------------------------===//
 // Methods of StreamChecker.
 //===----------------------------------------------------------------------===//
 
 void StreamChecker::checkPreCall(const CallEvent &Call,
                                  CheckerContext &C) const {
+  initEof(C);
+
   const FnDescription *Desc = lookupFn(Call);
   if (!Desc || !Desc->PreFn)
     return;
 
   Desc->PreFn(this, Desc, Call, C);
 }
 
 bool StreamChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
   // Bifurcate the state into two: one with a valid FILE* pointer, the other
   // with a NULL.
   ProgramStateRef StateNotNull, StateNull;
   std::tie(StateNotNull, StateNull) =
       C.getConstraintManager().assumeDual(State, RetVal);
 
   StateNotNull =
       StateNotNull->set<StreamMap>(RetSym, StreamState::getOpened(Desc));
+  StateNotNull = errno_modeling::setErrnoForStdSuccess(StateNotNull, C);
+
   StateNull =
       StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
+  StateNull = errno_modeling::setErrnoForStdFailure(StateNull, C,
+                                                    getErrnoSVal(C, Call));
 
-  C.addTransition(StateNotNull,
-                  constructNoteTag(C, RetSym, "Stream opened here"));
+  ExplodedNode *N = C.addTransition(
+      StateNotNull, constructNoteTag(C, RetSym, "Stream opened here"));
+  C.addTransition(N->getState(), N,
+                  errno_modeling::getNoteTagForStdSuccess(
+                      C, cast<NamedDecl>(Call.getDecl())->getNameAsString()));
   C.addTransition(StateNull);
 }
 
 void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call,
                                CheckerContext &C) const {
   // Do not allow NULL as passed stream pointer but allow a closed stream.
   ProgramStateRef State = C.getState();
   State = ensureStreamNonNull(getStreamArg(Desc, Call),
void StreamChecker::evalFreopen(const FnDescription *Desc,
   // Generate state for NULL return value.
   // Stream switches to OpenFailed state.
   ProgramStateRef StateRetNull =
       State->BindExpr(CE, C.getLocationContext(),
                       C.getSValBuilder().makeNullWithType(CE->getType()));
 
   StateRetNotNull =
       StateRetNotNull->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
+  StateRetNotNull = errno_modeling::setErrnoForStdSuccess(StateRetNotNull, C);
+
   StateRetNull =
       StateRetNull->set<StreamMap>(StreamSym, StreamState::getOpenFailed(Desc));
+  StateRetNull = errno_modeling::setErrnoForStdFailure(StateRetNull, C,
+                                                       getErrnoSVal(C, Call));
 
-  C.addTransition(StateRetNotNull,
-                  constructNoteTag(C, StreamSym, "Stream reopened here"));
+  ExplodedNode *N = C.addTransition(
+      StateRetNotNull, constructNoteTag(C, StreamSym, "Stream reopened here"));
+  C.addTransition(N->getState(), N,
+                  errno_modeling::getNoteTagForStdSuccess(C, "freopen"));
   C.addTransition(StateRetNull);
 }
 
 void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call,
                                CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SymbolRef Sym = getStreamArg(Desc, Call).getAsSymbol();
   if (!Sym)
     return;
 
   const StreamState *SS = State->get<StreamMap>(Sym);
   if (!SS)
     return;
 
+  auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return;
+
   assertStreamStateOpened(SS);
 
   // 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(Desc));
 
-  C.addTransition(State);
+  // Return 0 on success, EOF on failure.
+  SValBuilder &SVB = C.getSValBuilder();
+  ProgramStateRef StateSuccess = State->BindExpr(
+      CE, C.getLocationContext(), SVB.makeIntVal(0, C.getASTContext().IntTy));
+  ProgramStateRef StateFailure =
+      State->BindExpr(CE, C.getLocationContext(),
+                      SVB.makeIntVal(EofVal, C.getASTContext().IntTy));
+
+  StateSuccess = errno_modeling::setErrnoForStdSuccess(StateSuccess, C);
+  StateFailure = errno_modeling::setErrnoForStdFailure(StateFailure, C,
+                                                       getErrnoSVal(C, Call));
+
+  C.addTransition(StateSuccess,
+                  errno_modeling::getNoteTagForStdSuccess(C, "fclose"));
+  C.addTransition(StateFailure);
 }
 
 void StreamChecker::preFread(const FnDescription *Desc, const CallEvent &Call,
                              CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SVal StreamVal = getStreamArg(Desc, Call);
   State = ensureStreamNonNull(StreamVal, Call.getArgExpr(Desc->StreamArgNo), C,
                               State);
void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
   // size or nmemb is zero, fread returns zero and the contents of the array and
   // the state of the stream remain unchanged.
 
   if (State->isNull(*SizeVal).isConstrainedTrue() ||
       State->isNull(*NMembVal).isConstrainedTrue()) {
     // This is the "size or nmemb is zero" case.
     // Just return 0, do nothing more (not clear the error flags).
     State = bindInt(0, State, C, CE);
+    // It is unspecified what can happen with 'errno'.
+    State = errno_modeling::setErrnoState(State, errno_modeling::Irrelevant);
     C.addTransition(State);
     return;
   }
 
   // Generate a transition for the success state.
   // If we know the state to be FEOF at fread, do not add a success state.
   if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
     ProgramStateRef StateNotFailed =
         State->BindExpr(CE, C.getLocationContext(), *NMembVal);
     StateNotFailed =
         StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
-    C.addTransition(StateNotFailed);
+    StateNotFailed = errno_modeling::setErrnoForStdSuccess(StateNotFailed, C);
+    C.addTransition(StateNotFailed,
+                    errno_modeling::getNoteTagForStdSuccess(
+                        C, cast<NamedDecl>(Call.getDecl())->getNameAsString()));
   }
 
   // Add transition for the failed state.
   NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
   ProgramStateRef StateFailed =
       State->BindExpr(CE, C.getLocationContext(), RetVal);
   auto Cond =
       C.getSValBuilder()
if (IsFread)
     NewES =
         (OldSS->ErrorState == ErrorFEof) ? ErrorFEof : ErrorFEof | ErrorFError;
   else
     NewES = ErrorFError;
   // If a (non-EOF) error occurs, the resulting value of the file position
   // indicator for the stream is indeterminate.
   StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
   StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
+  StateFailed = errno_modeling::setErrnoForStdFailure(StateFailed, C,
+                                                      getErrnoSVal(C, Call));
   if (IsFread && OldSS->ErrorState != ErrorFEof)
     C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
   else
     C.addTransition(StateFailed);
 }
 
 void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call,
                              CheckerContext &C) const {
void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
   // We get error.
   // It is possible that fseek fails but sets none of the error flags.
   // If fseek failed, assume that the file position becomes indeterminate in any
   // case.
   StateFailed = StateFailed->set<StreamMap>(
       StreamSym,
       StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true));
 
-  C.addTransition(StateNotFailed);
+  StateNotFailed = errno_modeling::setErrnoForStdSuccess(StateNotFailed, C);
+  StateFailed = errno_modeling::setErrnoForStdFailure(StateFailed, C,
+                                                      getErrnoSVal(C, Call));
+
+  C.addTransition(StateNotFailed,
+                  errno_modeling::getNoteTagForStdSuccess(C, "fseek"));
   C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
 }
 
 void StreamChecker::evalClearerr(const FnDescription *Desc,
                                  const CallEvent &Call,
                                  CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();

clang/test/Analysis/stream-errno.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,alpha.unix.Errno,debug.ExprInspection -analyzer-output text -verify %s
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/errno_func.h"
+
+extern void clang_analyzer_eval(int);
+extern void clang_analyzer_dump(int);
+
+void check_fopen(void) {
+  FILE *F = fopen("xxx", "r");
+  // expected-note@-1{{Assuming that function 'fopen' is successful, in this case the value 'errno' may be undefined after the call and should not be used}}
+  // expected-note@+4{{'F' is null}}
+  // expected-note@+3{{Taking true branch}}
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F) {
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}} expected-note{{TRUE}}
+    if (errno) {}
+    return;
+  }
+  if (errno) {} // expected-warning{{An undefined value may be read from 'errno' [alpha.unix.Errno]}}
+  // expected-note@-1{{An undefined value may be read from 'errno'}}
+}
+
+void check_tmpfile(void) {
+  FILE *F = tmpfile();
+  // expected-note@-1{{Assuming that function 'tmpfile' is successful, in this case the value 'errno' may be undefined after the call and should not be used}}
+  // expected-note@+4{{'F' is null}}
+  // expected-note@+3{{Taking true branch}}
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F) {
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}} expected-note{{TRUE}}
+    if (errno) {}
+    return;
+  }
+  if (errno) {} // expected-warning{{An undefined value may be read from 'errno' [alpha.unix.Errno]}}
+  // expected-note@-1{{An undefined value may be read from 'errno'}}
+}
+
+void check_freopen(void) {
+  FILE *F = tmpfile();
+  // expected-note@+4{{'F' is non-null}}
+  // expected-note@+3{{Taking false branch}}
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F)
+    return;
+  F = freopen("xxx", "w", F);
+  // expected-note@-1{{Assuming that function 'freopen' is successful, in this case the value 'errno' may be undefined after the call and should not be used}}
+  // expected-note@+4{{'F' is null}}
+  // expected-note@+3{{Taking true branch}}
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F) {
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}} expected-note{{TRUE}}
+    if (errno) {}
+    return;
+  }
+  if (errno) {} // expected-warning{{An undefined value may be read from 'errno' [alpha.unix.Errno]}}
+  // expected-note@-1{{An undefined value may be read from 'errno'}}
+}
+
+void check_fclose(void) {
+  FILE *F = tmpfile();
+  // expected-note@+4{{'F' is non-null}}
+  // expected-note@+3{{Taking false branch}}
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F)
+    return;
+  int Ret = fclose(F);
+  // expected-note@-1{{Assuming that function 'fclose' is successful, in this case the value 'errno' may be undefined after the call and should not be used}}
+  // expected-note@+2{{Taking true branch}}
+  // expected-note@+1{{Taking false branch}}
+  if (Ret == EOF) {
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}} expected-note{{TRUE}}
+    if (errno) {}
+    return;
+  }
+  if (errno) {} // expected-warning{{An undefined value may be read from 'errno' [alpha.unix.Errno]}}
+  // expected-note@-1{{An undefined value may be read from 'errno'}}
+}
+
+void check_fread(void) {
+  char Buf[10];
+  FILE *F = tmpfile();
+  // expected-note@+4{{'F' is non-null}}
+  // expected-note@+3{{Taking false branch}}
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F)
+    return;
+  fread(Buf, 0, 1, F);
+  // expected-note@+2{{Taking false branch}}
+  // expected-note@+1{{Taking false branch}}
+  if (errno) {} // no-warning
+  fread(Buf, 1, 0, F);
+  // expected-note@+2{{Taking false branch}}
+  // expected-note@+1{{Taking false branch}}
+  if (errno) {} // no-warning
+
+  int R = fread(Buf, 1, 10, F);
+  // expected-note@-1{{function 'fread' is successful}}
+  // expected-note@+4{{'R' is < 10}}
+  // expected-note@+3{{Taking true branch}}
+  // expected-note@+2{{'R' is >= 10}}
+  // expected-note@+1{{Taking false branch}}
+  if (R < 10) {
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}} expected-note{{TRUE}}
+    if (errno) {}
+    fclose(F);
+    return;
+  }
+  if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
+  // expected-note@-1{{An undefined value may be read from 'errno'}}
+}
+
+void check_fwrite(void) {
+  char Buf[] = "0123456789";
+  FILE *F = tmpfile();
+  // expected-note@+4{{'F' is non-null}}
+  // expected-note@+3{{Taking false branch}}
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F)
+    return;
+  fwrite(Buf, 0, 1, F);
+  // expected-note@+2{{Taking false branch}}
+  // expected-note@+1{{Taking false branch}}
+  if (errno) {} // no-warning
+  fwrite(Buf, 1, 0, F);
+  // expected-note@+2{{Taking false branch}}
+  // expected-note@+1{{Taking false branch}}
+  if (errno) {} // no-warning
+
+  int R = fwrite(Buf, 1, 10, F);
+  // expected-note@-1{{function 'fwrite' is successful}}
+  // expected-note@+4{{'R' is < 10}}
+  // expected-note@+3{{Taking true branch}}
+  // expected-note@+2{{'R' is >= 10}}
+  // expected-note@+1{{Taking false branch}}
+  if (R < 10) {
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}} expected-note{{TRUE}}
+    if (errno) {}
+    fclose(F);
+    return;
+  }
+  if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
+  // expected-note@-1{{An undefined value may be read from 'errno'}}
+}
+
+void check_fseek(void) {
+  FILE *F = tmpfile();
+  // expected-note@+4{{'F' is non-null}}
+  // expected-note@+3{{Taking false branch}}
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F)
+    return;
+  int S = fseek(F, 11, SEEK_SET);
+  // expected-note@-1{{Assuming that function 'fseek' is successful}}
+  // expected-note@+4{{'S' is not equal to 0}}
+  // expected-note@+3{{Taking true branch}}
+  // expected-note@+2{{'S' is equal to 0}}
+  // expected-note@+1{{Taking false branch}}
+  if (S != 0) {
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}} expected-note{{TRUE}}
+    if (errno) {}
+    fclose(F);
+    return;
+  }
+  if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
+  // expected-note@-1{{An undefined value may be read from 'errno'}}
+}
+
+void check_no_errno_change(void) {
+  FILE *F = tmpfile();
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F)
+    return;
+  errno = 1;
+  clearerr(F);
+  // expected-note@+1{{Taking true branch}}
+  if (errno) {} // no-warning
+  feof(F);
+  // expected-note@+1{{Taking true branch}}
+  if (errno) {} // no-warning
+  ferror(F);
+  // expected-note@+1{{Taking true branch}}
+  if (errno) {} // no-warning
+  clang_analyzer_eval(errno == 1); // expected-warning{{TRUE}} expected-note{{TRUE}}
+  fclose(F);
+}
+