Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[clang][analyzer] Improve modeling of fileno in the StreamChecker #76207

Closed
wants to merge 1 commit into from
Closed

[clang][analyzer] Improve modeling of fileno in the StreamChecker #76207

wants to merge 1 commit into from

Conversation

benshi001
Copy link
Member

No description provided.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Dec 22, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 22, 2023

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

Author: Ben Shi (benshi001)

Changes

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

3 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+19-11)
  • (modified) clang/test/Analysis/stream-errno.c (+4-3)
  • (modified) clang/test/Analysis/stream-noopen.c (+12)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 254b36ed03968d..80ac910060720f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -265,7 +265,11 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
       {{{"fseek"}, 3},
        {&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}},
       {{{"ftell"}, 1},
-       {&StreamChecker::preDefault, &StreamChecker::evalFtell, 0}},
+       {&StreamChecker::preDefault,
+        std::bind(&StreamChecker::evalFtellFileno, _1, _2, _3, _4, true), 0}},
+      {{{"fileno"}, 1},
+       {&StreamChecker::preDefault,
+        std::bind(&StreamChecker::evalFtellFileno, _1, _2, _3, _4, false), 0}},
       {{{"fflush"}, 1},
        {&StreamChecker::preFflush, &StreamChecker::evalFflush, 0}},
       {{{"rewind"}, 1},
@@ -284,7 +288,6 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
        {&StreamChecker::preDefault,
         std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFError),
         0}},
-      {{{"fileno"}, 1}, {&StreamChecker::preDefault, nullptr, 0}},
   };
 
   CallDescriptionMap<FnDescription> FnTestDescriptions = {
@@ -342,8 +345,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   void evalFsetpos(const FnDescription *Desc, const CallEvent &Call,
                    CheckerContext &C) const;
 
-  void evalFtell(const FnDescription *Desc, const CallEvent &Call,
-                 CheckerContext &C) const;
+  void evalFtellFileno(const FnDescription *Desc, const CallEvent &Call,
+                       CheckerContext &C, bool IsRetLongTy) const;
 
   void evalRewind(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
@@ -1049,8 +1052,9 @@ void StreamChecker::evalFsetpos(const FnDescription *Desc,
   C.addTransition(StateFailed);
 }
 
-void StreamChecker::evalFtell(const FnDescription *Desc, const CallEvent &Call,
-                              CheckerContext &C) const {
+void StreamChecker::evalFtellFileno(const FnDescription *Desc,
+                                    const CallEvent &Call, CheckerContext &C,
+                                    bool IsRetLongTy) const {
   ProgramStateRef State = C.getState();
   SymbolRef Sym = getStreamArg(Desc, Call).getAsSymbol();
   if (!Sym)
@@ -1067,10 +1071,12 @@ void StreamChecker::evalFtell(const FnDescription *Desc, const CallEvent &Call,
   NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
   ProgramStateRef StateNotFailed =
       State->BindExpr(CE, C.getLocationContext(), RetVal);
-  auto Cond = SVB.evalBinOp(State, BO_GE, RetVal,
-                            SVB.makeZeroVal(C.getASTContext().LongTy),
-                            SVB.getConditionType())
-                  .getAs<DefinedOrUnknownSVal>();
+  auto Cond =
+      SVB.evalBinOp(State, BO_GE, RetVal,
+                    SVB.makeZeroVal(IsRetLongTy ? C.getASTContext().LongTy
+                                                : C.getASTContext().IntTy),
+                    SVB.getConditionType())
+          .getAs<DefinedOrUnknownSVal>();
   if (!Cond)
     return;
   StateNotFailed = StateNotFailed->assume(*Cond, true);
@@ -1078,7 +1084,9 @@ void StreamChecker::evalFtell(const FnDescription *Desc, const CallEvent &Call,
     return;
 
   ProgramStateRef StateFailed = State->BindExpr(
-      CE, C.getLocationContext(), SVB.makeIntVal(-1, C.getASTContext().LongTy));
+      CE, C.getLocationContext(),
+      SVB.makeIntVal(-1, IsRetLongTy ? C.getASTContext().LongTy
+                                     : C.getASTContext().IntTy));
 
   // This function does not affect the stream state.
   // Still we add success and failure state with the appropriate return value.
diff --git a/clang/test/Analysis/stream-errno.c b/clang/test/Analysis/stream-errno.c
index bf0a61db2424f9..62befbc78e5a75 100644
--- a/clang/test/Analysis/stream-errno.c
+++ b/clang/test/Analysis/stream-errno.c
@@ -216,9 +216,10 @@ void check_fileno(void) {
   int N = fileno(F);
   if (N == -1) {
     clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
-    if (errno) {} // no-warning
-    fclose(F);
-    return;
+    if (errno) {}                    // no-warning
+  } else {
+    clang_analyzer_eval(N >= 0);     // expected-warning{{TRUE}}
   }
   if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
+  fclose(F);
 }
diff --git a/clang/test/Analysis/stream-noopen.c b/clang/test/Analysis/stream-noopen.c
index 2daf640c18a1d4..198b5dc45e35da 100644
--- a/clang/test/Analysis/stream-noopen.c
+++ b/clang/test/Analysis/stream-noopen.c
@@ -129,6 +129,18 @@ void check_ftell(FILE *F) {
   clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
 }
 
+void check_fileno(FILE *F) {
+  int Ret = fileno(F);
+  clang_analyzer_eval(F != NULL);    // expected-warning{{TRUE}}
+  if (!(Ret >= 0)) {
+    clang_analyzer_eval(Ret == -1);  // expected-warning{{TRUE}}
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+    if (errno) {}                    // no-warning
+  }
+  clang_analyzer_eval(feof(F));      // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(ferror(F));    // expected-warning{{UNKNOWN}}
+}
+
 void test_rewind(FILE *F) {
   errno = 0;
   rewind(F);

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 22, 2023

@llvm/pr-subscribers-clang

Author: Ben Shi (benshi001)

Changes

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

3 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+19-11)
  • (modified) clang/test/Analysis/stream-errno.c (+4-3)
  • (modified) clang/test/Analysis/stream-noopen.c (+12)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 254b36ed03968d..80ac910060720f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -265,7 +265,11 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
       {{{"fseek"}, 3},
        {&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}},
       {{{"ftell"}, 1},
-       {&StreamChecker::preDefault, &StreamChecker::evalFtell, 0}},
+       {&StreamChecker::preDefault,
+        std::bind(&StreamChecker::evalFtellFileno, _1, _2, _3, _4, true), 0}},
+      {{{"fileno"}, 1},
+       {&StreamChecker::preDefault,
+        std::bind(&StreamChecker::evalFtellFileno, _1, _2, _3, _4, false), 0}},
       {{{"fflush"}, 1},
        {&StreamChecker::preFflush, &StreamChecker::evalFflush, 0}},
       {{{"rewind"}, 1},
@@ -284,7 +288,6 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
        {&StreamChecker::preDefault,
         std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFError),
         0}},
-      {{{"fileno"}, 1}, {&StreamChecker::preDefault, nullptr, 0}},
   };
 
   CallDescriptionMap<FnDescription> FnTestDescriptions = {
@@ -342,8 +345,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   void evalFsetpos(const FnDescription *Desc, const CallEvent &Call,
                    CheckerContext &C) const;
 
-  void evalFtell(const FnDescription *Desc, const CallEvent &Call,
-                 CheckerContext &C) const;
+  void evalFtellFileno(const FnDescription *Desc, const CallEvent &Call,
+                       CheckerContext &C, bool IsRetLongTy) const;
 
   void evalRewind(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
@@ -1049,8 +1052,9 @@ void StreamChecker::evalFsetpos(const FnDescription *Desc,
   C.addTransition(StateFailed);
 }
 
-void StreamChecker::evalFtell(const FnDescription *Desc, const CallEvent &Call,
-                              CheckerContext &C) const {
+void StreamChecker::evalFtellFileno(const FnDescription *Desc,
+                                    const CallEvent &Call, CheckerContext &C,
+                                    bool IsRetLongTy) const {
   ProgramStateRef State = C.getState();
   SymbolRef Sym = getStreamArg(Desc, Call).getAsSymbol();
   if (!Sym)
@@ -1067,10 +1071,12 @@ void StreamChecker::evalFtell(const FnDescription *Desc, const CallEvent &Call,
   NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
   ProgramStateRef StateNotFailed =
       State->BindExpr(CE, C.getLocationContext(), RetVal);
-  auto Cond = SVB.evalBinOp(State, BO_GE, RetVal,
-                            SVB.makeZeroVal(C.getASTContext().LongTy),
-                            SVB.getConditionType())
-                  .getAs<DefinedOrUnknownSVal>();
+  auto Cond =
+      SVB.evalBinOp(State, BO_GE, RetVal,
+                    SVB.makeZeroVal(IsRetLongTy ? C.getASTContext().LongTy
+                                                : C.getASTContext().IntTy),
+                    SVB.getConditionType())
+          .getAs<DefinedOrUnknownSVal>();
   if (!Cond)
     return;
   StateNotFailed = StateNotFailed->assume(*Cond, true);
@@ -1078,7 +1084,9 @@ void StreamChecker::evalFtell(const FnDescription *Desc, const CallEvent &Call,
     return;
 
   ProgramStateRef StateFailed = State->BindExpr(
-      CE, C.getLocationContext(), SVB.makeIntVal(-1, C.getASTContext().LongTy));
+      CE, C.getLocationContext(),
+      SVB.makeIntVal(-1, IsRetLongTy ? C.getASTContext().LongTy
+                                     : C.getASTContext().IntTy));
 
   // This function does not affect the stream state.
   // Still we add success and failure state with the appropriate return value.
diff --git a/clang/test/Analysis/stream-errno.c b/clang/test/Analysis/stream-errno.c
index bf0a61db2424f9..62befbc78e5a75 100644
--- a/clang/test/Analysis/stream-errno.c
+++ b/clang/test/Analysis/stream-errno.c
@@ -216,9 +216,10 @@ void check_fileno(void) {
   int N = fileno(F);
   if (N == -1) {
     clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
-    if (errno) {} // no-warning
-    fclose(F);
-    return;
+    if (errno) {}                    // no-warning
+  } else {
+    clang_analyzer_eval(N >= 0);     // expected-warning{{TRUE}}
   }
   if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
+  fclose(F);
 }
diff --git a/clang/test/Analysis/stream-noopen.c b/clang/test/Analysis/stream-noopen.c
index 2daf640c18a1d4..198b5dc45e35da 100644
--- a/clang/test/Analysis/stream-noopen.c
+++ b/clang/test/Analysis/stream-noopen.c
@@ -129,6 +129,18 @@ void check_ftell(FILE *F) {
   clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
 }
 
+void check_fileno(FILE *F) {
+  int Ret = fileno(F);
+  clang_analyzer_eval(F != NULL);    // expected-warning{{TRUE}}
+  if (!(Ret >= 0)) {
+    clang_analyzer_eval(Ret == -1);  // expected-warning{{TRUE}}
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+    if (errno) {}                    // no-warning
+  }
+  clang_analyzer_eval(feof(F));      // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(ferror(F));    // expected-warning{{UNKNOWN}}
+}
+
 void test_rewind(FILE *F) {
   errno = 0;
   rewind(F);

@benshi001
Copy link
Member Author

fileno and ftell are quite similar,

  1. both of them return 0 on success, and -1 on failure.
  2. both of them set errno on failure.

The differences are

  1. fileno returns int type but ftell returns long type.
  2. ftell will not affect the value of error on success, but this is not mentioned for fileno.

@balazske
Copy link
Collaborator

I do not see much benefit of adding fileno to the checker in the current state. If the StdLibraryFunctionsChecker is turned on it will anyway do a similar modeling of fileno (this applied to ftell too). The fileno and ftell are semantically different and fileno can be more complicated if we want to ensure that the returned file numbers are different from each other, and special values for stdin, stdout, stderr should be taken into account.
Adding fileno in the current way makes a small improvement for the case when StdLibraryFunctionsChecker is not used. A combined function for fileno and ftell can be used, but in a way that works with any function that returns -1 on error and >=0 value on success. The type of the return value can be get from the declaration of the function or from the CallEvent in some way, a type parameter to the function is not needed.
Adding the standard streams to the checker has some difficulties and requires more work (at least there must be a checker parameter to assume that standard streams are not changed by the program, otherwise no modeling can be done because these are global variables).

@benshi001 benshi001 closed this Dec 22, 2023
@benshi001 benshi001 deleted the csa-fileno branch December 22, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steakhal steakhal Awaiting requested review from steakhal

@balazske balazske Awaiting requested review from balazske

Assignees
No one assigned
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@benshi001 @llvmbot @balazske