[clang] Reapply "[clang][analyzer] StreamChecker: Model getc, vfscanf, putc, … (PR #83281)

via cfe-commits cfe-commits at lists.llvm.org
Wed Feb 28 07:46:43 PST 2024


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: Alejandro Álvarez Ayllón (alejandro-alvarez-sonarsource)

<details>
<summary>Changes</summary>

…vfprintf (#<!-- -->82476)"

`va_list` is a platform-specific type. On some, it is a struct instead of a pointer to a struct, so `lookupFn` was ignoring calls to `vfprintf` and `vfscanf`.

`stream.c` now runs in four different platforms to make sure the logic works across targets.

This reverts commit 570bc5d291f92e19f6264262b02ddff1a2f2e09b.



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


6 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+29-5) 
- (modified) clang/test/Analysis/Inputs/system-header-simulator-for-simple-stream.h (+1-1) 
- (modified) clang/test/Analysis/Inputs/system-header-simulator-for-valist.h (+6) 
- (modified) clang/test/Analysis/Inputs/system-header-simulator.h (+3) 
- (modified) clang/test/Analysis/stream-invalidate.c (+42) 
- (modified) clang/test/Analysis/stream.c (+10-331) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 65bdc4cac30940..f9928e1325c53d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -348,18 +348,30 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
       {{{"fgets"}, 3},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
         std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, false), 2}},
+      {{{"getc"}, 1},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+        std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, true), 0}},
       {{{"fputc"}, 2},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
         std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, true), 1}},
       {{{"fputs"}, 2},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
         std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, false), 1}},
+      {{{"putc"}, 2},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
+        std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, true), 1}},
       {{{"fprintf"}},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
         std::bind(&StreamChecker::evalFprintf, _1, _2, _3, _4), 0}},
+      {{{"vfprintf"}, 3},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
+        std::bind(&StreamChecker::evalFprintf, _1, _2, _3, _4), 0}},
       {{{"fscanf"}},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
         std::bind(&StreamChecker::evalFscanf, _1, _2, _3, _4), 0}},
+      {{{"vfscanf"}, 3},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+        std::bind(&StreamChecker::evalFscanf, _1, _2, _3, _4), 0}},
       {{{"ungetc"}, 2},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
         std::bind(&StreamChecker::evalUngetc, _1, _2, _3, _4), 1}},
@@ -419,6 +431,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   mutable int SeekCurVal = 1;
   /// Expanded value of SEEK_END, 2 if not found.
   mutable int SeekEndVal = 2;
+  /// The built-in va_list type is platform-specific
+  mutable QualType VaListType;
 
   void evalFopen(const FnDescription *Desc, const CallEvent &Call,
                  CheckerContext &C) const;
@@ -548,7 +562,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
       return nullptr;
     for (auto *P : Call.parameters()) {
       QualType T = P->getType();
-      if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
+      if (!T->isIntegralOrEnumerationType() && !T->isPointerType() &&
+          T.getCanonicalType() != VaListType)
         return nullptr;
     }
 
@@ -600,6 +615,11 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
       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,
@@ -652,6 +672,7 @@ 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)
@@ -1038,10 +1059,13 @@ void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call,
     if (!StateNotFailed)
       return;
 
-    SmallVector<unsigned int> EscArgs;
-    for (auto EscArg : llvm::seq(2u, Call.getNumArgs()))
-      EscArgs.push_back(EscArg);
-    StateNotFailed = escapeArgs(StateNotFailed, C, Call, EscArgs);
+    if (auto const *Callee = Call.getCalleeIdentifier();
+        !Callee || !Callee->getName().equals("vfscanf")) {
+      SmallVector<unsigned int> EscArgs;
+      for (auto EscArg : llvm::seq(2u, Call.getNumArgs()))
+        EscArgs.push_back(EscArg);
+      StateNotFailed = escapeArgs(StateNotFailed, C, Call, EscArgs);
+    }
 
     if (StateNotFailed)
       C.addTransition(StateNotFailed);
diff --git a/clang/test/Analysis/Inputs/system-header-simulator-for-simple-stream.h b/clang/test/Analysis/Inputs/system-header-simulator-for-simple-stream.h
index 098a2208fecbe9..c26d3582149120 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator-for-simple-stream.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator-for-simple-stream.h
@@ -5,7 +5,7 @@
 // suppressed.
 #pragma clang system_header
 
-typedef struct __sFILE {
+typedef struct _FILE {
   unsigned char *_p;
 } FILE;
 FILE *fopen(const char *restrict, const char *restrict) __asm("_" "fopen" );
diff --git a/clang/test/Analysis/Inputs/system-header-simulator-for-valist.h b/clang/test/Analysis/Inputs/system-header-simulator-for-valist.h
index 7299b61353d460..720944abb8ad47 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator-for-valist.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator-for-valist.h
@@ -10,6 +10,8 @@
 #define restrict /*restrict*/
 #endif
 
+typedef struct _FILE FILE;
+
 typedef __builtin_va_list va_list;
 
 #define va_start(ap, param) __builtin_va_start(ap, param)
@@ -21,6 +23,10 @@ int vprintf (const char *restrict format, va_list arg);
 
 int vsprintf (char *restrict s, const char *restrict format, va_list arg);
 
+int vfprintf(FILE *stream, const char *format, va_list ap);
+
+int vfscanf(FILE *stream, const char *format, va_list ap);
+
 int some_library_function(int n, va_list arg);
 
 // No warning from system header.
diff --git a/clang/test/Analysis/Inputs/system-header-simulator.h b/clang/test/Analysis/Inputs/system-header-simulator.h
index 15986984802c0e..8fd51449ecc0a4 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator.h
@@ -73,6 +73,9 @@ int ferror(FILE *stream);
 int fileno(FILE *stream);
 int fflush(FILE *stream);
 
+
+int getc(FILE *stream);
+
 size_t strlen(const char *);
 
 char *strcpy(char *restrict, const char *restrict);
diff --git a/clang/test/Analysis/stream-invalidate.c b/clang/test/Analysis/stream-invalidate.c
index 6745d11a2fe701..5046a356d0583d 100644
--- a/clang/test/Analysis/stream-invalidate.c
+++ b/clang/test/Analysis/stream-invalidate.c
@@ -4,6 +4,7 @@
 // RUN: -analyzer-checker=debug.ExprInspection
 
 #include "Inputs/system-header-simulator.h"
+#include "Inputs/system-header-simulator-for-valist.h"
 
 void clang_analyzer_eval(int);
 void clang_analyzer_dump(int);
@@ -145,3 +146,44 @@ void test_fgetpos() {
 
   fclose(F);
 }
+
+void test_fprintf() {
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return;
+
+  unsigned a = 42;
+  char *output = "HELLO";
+  int r = fprintf(F1, "%s\t%u\n", output, a);
+  // fprintf does not invalidate any of its input
+  // 69 is ascii for 'E'
+  clang_analyzer_dump(a); // expected-warning {{42 S32b}}
+  clang_analyzer_dump(output[1]); // expected-warning {{69 S32b}}
+  fclose(F1);
+}
+
+int test_vfscanf_inner(const char *fmt, ...) {
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return EOF;
+
+  va_list ap;
+  va_start(ap, fmt);
+
+  int r = vfscanf(F1, fmt, ap);
+
+  fclose(F1);
+  va_end(ap);
+  return r;
+}
+
+void test_vfscanf() {
+  int i = 42;
+  int j = 43;
+  int r = test_vfscanf_inner("%d", &i);
+  if (r != EOF) {
+    // i gets invalidated by the call to test_vfscanf_inner, not by vfscanf.
+    clang_analyzer_dump(i); // expected-warning {{conj_$}}
+    clang_analyzer_dump(j); // expected-warning {{43 S32b}}
+  }
+}
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index 378c9154f8f6a8..40b76c7dccb5bc 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -1,341 +1,20 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -triple=x86_64-pc-linux-gnu -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -triple=armv8-none-linux-eabi -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -triple=aarch64-linux-gnu -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -triple=hexagon -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
 
 #include "Inputs/system-header-simulator.h"
+#include "Inputs/system-header-simulator-for-valist.h"
 
-void clang_analyzer_eval(int);
 
-void check_fread(void) {
-  FILE *fp = tmpfile();
-  fread(0, 0, 0, fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_fwrite(void) {
-  FILE *fp = tmpfile();
-  fwrite(0, 0, 0, fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_fgetc(void) {
-  FILE *fp = tmpfile();
-  fgetc(fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_fgets(void) {
-  FILE *fp = tmpfile();
-  char buf[256];
-  fgets(buf, sizeof(buf), fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_fputc(void) {
-  FILE *fp = tmpfile();
-  fputc('A', fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_fputs(void) {
-  FILE *fp = tmpfile();
-  fputs("ABC", fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_fprintf(void) {
-  FILE *fp = tmpfile();
-  fprintf(fp, "ABC"); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_fscanf(void) {
-  FILE *fp = tmpfile();
-  fscanf(fp, "ABC"); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_ungetc(void) {
-  FILE *fp = tmpfile();
-  ungetc('A', fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_fseek(void) {
-  FILE *fp = tmpfile();
-  fseek(fp, 0, 0); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_ftell(void) {
-  FILE *fp = tmpfile();
-  ftell(fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_rewind(void) {
-  FILE *fp = tmpfile();
-  rewind(fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_fgetpos(void) {
-  FILE *fp = tmpfile();
-  fpos_t pos;
-  fgetpos(fp, &pos); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_fsetpos(void) {
-  FILE *fp = tmpfile();
-  fpos_t pos;
-  fsetpos(fp, &pos); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_clearerr(void) {
-  FILE *fp = tmpfile();
-  clearerr(fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_feof(void) {
-  FILE *fp = tmpfile();
-  feof(fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_ferror(void) {
-  FILE *fp = tmpfile();
-  ferror(fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void check_fileno(void) {
-  FILE *fp = tmpfile();
-  fileno(fp); // expected-warning {{Stream pointer might be NULL}}
-  fclose(fp);
-}
-
-void f_open(void) {
-  FILE *p = fopen("foo", "r");
-  char buf[1024];
-  fread(buf, 1, 1, p); // expected-warning {{Stream pointer might be NULL}}
-  fclose(p);
-}
-
-void f_dopen(int fd) {
+void f_vfprintf(int fd, va_list args) {
   FILE *F = fdopen(fd, "r");
-  char buf[1024];
-  fread(buf, 1, 1, F); // expected-warning {{Stream pointer might be NULL}}
+  vfprintf(F, "%d", args); // expected-warning {{Stream pointer might be NULL}}
   fclose(F);
 }
 
-void f_seek(void) {
-  FILE *p = fopen("foo", "r");
-  if (!p)
-    return;
-  fseek(p, 1, SEEK_SET); // no-warning
-  fseek(p, 1, 3); // expected-warning {{The whence argument to fseek() should be SEEK_SET, SEEK_END, or SEEK_CUR}}
-  fclose(p);
-}
-
-void f_double_close(void) {
-  FILE *p = fopen("foo", "r");
-  if (!p)
-    return;
-  fclose(p);
-  fclose(p); // expected-warning {{Stream might be already closed}}
-}
-
-void f_double_close_alias(void) {
-  FILE *p1 = fopen("foo", "r");
-  if (!p1)
-    return;
-  FILE *p2 = p1;
-  fclose(p1);
-  fclose(p2); // expected-warning {{Stream might be already closed}}
-}
-
-void f_use_after_close(void) {
-  FILE *p = fopen("foo", "r");
-  if (!p)
-    return;
-  fclose(p);
-  clearerr(p); // expected-warning {{Stream might be already closed}}
-}
-
-void f_open_after_close(void) {
-  FILE *p = fopen("foo", "r");
-  if (!p)
-    return;
-  fclose(p);
-  p = fopen("foo", "r");
-  if (!p)
-    return;
-  fclose(p);
-}
-
-void f_reopen_after_close(void) {
-  FILE *p = fopen("foo", "r");
-  if (!p)
-    return;
-  fclose(p);
-  // Allow reopen after close.
-  p = freopen("foo", "w", p);
-  if (!p)
-    return;
-  fclose(p);
-}
-
-void f_leak(int c) {
-  FILE *p = fopen("foo.c", "r");
-  if (!p)
-    return;
-  if(c)
-    return; // expected-warning {{Opened stream never closed. Potential resource leak}}
-  fclose(p);
-}
-
-FILE *f_null_checked(void) {
-  FILE *p = fopen("foo.c", "r");
-  if (p)
-    return p; // no-warning
-  else
-    return 0;
-}
-
-void pr7831(FILE *fp) {
-  fclose(fp); // no-warning
-}
-
-// PR 8081 - null pointer crash when 'whence' is not an integer constant
-void pr8081(FILE *stream, long offset, int whence) {
-  fseek(stream, offset, whence);
-}
-
-void check_freopen_1(void) {
-  FILE *f1 = freopen("foo.c", "r", (FILE *)0); // expected-warning {{Stream pointer might be NULL}}
-  f1 = freopen(0, "w", (FILE *)0x123456);      // Do not report this as error.
-}
-
-void check_freopen_2(void) {
-  FILE *f1 = fopen("foo.c", "r");
-  if (f1) {
-    FILE *f2 = freopen(0, "w", f1);
-    if (f2) {
-      // Check if f1 and f2 point to the same stream.
-      fclose(f1);
-      fclose(f2); // expected-warning {{Stream might be already closed.}}
-    } else {
-      // Reopen failed.
-      // f1 is non-NULL but points to a possibly invalid stream.
-      rewind(f1); // expected-warning {{Stream might be invalid}}
-      // f2 is NULL but the previous error stops the checker.
-      rewind(f2);
-    }
-  }
-}
-
-void check_freopen_3(void) {
-  FILE *f1 = fopen("foo.c", "r");
-  if (f1) {
-    // Unchecked result of freopen.
-    // The f1 may be invalid after this call.
-    freopen(0, "w", f1);
-    rewind(f1); // expected-warning {{Stream might be invalid}}
-    fclose(f1);
-  }
-}
-
-extern FILE *GlobalF;
-extern void takeFile(FILE *);
-
-void check_escape1(void) {
-  FILE *F = tmpfile();
-  if (!F)
-    return;
-  fwrite("1", 1, 1, F); // may fail
-  GlobalF = F;
-  fwrite("1", 1, 1, F); // no warning
-}
-
-void check_escape2(void) {
-  FILE *F = tmpfile();
-  if (!F)
-    return;
-  fwrite("1", 1, 1, F); // may fail
-  takeFile(F);
-  fwrite("1", 1, 1, F); // no warning
-}
-
-void check_escape3(void) {
-  FILE *F = tmpfile();
-  if (!F)
-    return;
-  takeFile(F);
-  F = freopen(0, "w", F);
-  if (!F)
-    return;
-  fwrite("1", 1, 1, F); // may fail
-  fwrite("1", 1, 1, F); // no warning
-}
-
-void check_escape4(void) {
-  FILE *F = tmpfile();
-  if (!F)
-    return;
-  fwrite("1", 1, 1, F); // may fail
-
-  // no escape at a non-StreamChecker-handled system call
-  setbuf(F, "0");
-
-  fwrite("1", 1, 1, F); // expected-warning {{might be 'indeterminate'}}
-  fclose(F);
-}
-
-int Test;
-_Noreturn void handle_error(void);
-
-void check_leak_noreturn_1(void) {
-  FILE *F1 = tmpfile();
-  if (!F1)
-    return;
-  if (Test == 1) {
-    handle_error(); // no warning
-  }
-  rewind(F1);
-} // expected-warning {{Opened stream never closed. Potential resource leak}}
-
-// Check that "location uniqueing" works.
-// This results in reporting only one occurence of resource leak for a stream.
-void check_leak_noreturn_2(void) {
-  FILE *F1 = tmpfile();
-  if (!F1)
-    return;
-  if (Test == 1) {
-    return; // no warning
-  }
-  rewind(F1);
-} // expected-warning {{Opened stream never closed. Potential resource leak}}
-// FIXME: This warning should be placed at the `return` above.
-// See https://reviews.llvm.org/D83120 about details.
-
-void fflush_after_fclose(void) {
-  FILE *F = tmpfile();
-  int Ret;
-  fflush(NULL);                      // no-warning
-  if (!F)
-    return;
-  if ((Ret = fflush(F)) != 0)
-    clang_analyzer_eval(Ret == EOF); // expected-warning {{TRUE}}
-  fclose(F);
-  fflush(F);                         // expected-warning {{Stream might be already closed}}
-}
-
-void fflush_on_open_failed_stream(void) {
-  FILE *F = tmpfile();
-  if (!F) {
-    fflush(F); // no-warning
-    return;
-  }
+void f_vfscanf(int fd, va_list args) {
+  FILE *F = fdopen(fd, "r");
+  vfscanf(F, "%u", args); // expected-warning {{Stream pointer might be NULL}}
   fclose(F);
 }

``````````

</details>


https://github.com/llvm/llvm-project/pull/83281


More information about the cfe-commits mailing list