[clang] [clang][analyzer] StreamChecker: Model getc, vfscanf, putc, vfprintf (PR #82476)

Alejandro Álvarez Ayllón via cfe-commits cfe-commits at lists.llvm.org
Mon Feb 26 08:36:54 PST 2024


https://github.com/alejandro-alvarez-sonarsource updated https://github.com/llvm/llvm-project/pull/82476

>From a21881d82fe3674b344d4a3807e9d2590c98ce93 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
 <alejandro.alvarez at sonarsource.com>
Date: Tue, 14 Nov 2023 09:28:45 +0100
Subject: [PATCH 01/10] [clang][analyzer] StreamChecker: add more APIs,
 invalidate fscanf args

1. Model getc, vfscanf, putc, vfprintf.
2. fscanf invalidates all arguments after the format string.
---
 .../StaticAnalyzer/Checkers/StreamChecker.cpp |  39 +++++-
 ...ystem-header-simulator-for-simple-stream.h |   2 +-
 .../system-header-simulator-for-valist.h      |   4 +
 .../Analysis/Inputs/system-header-simulator.h |   3 +
 clang/test/Analysis/stream.c                  | 128 ++++++++++++++++++
 5 files changed, 174 insertions(+), 2 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index a070f451694a3b..7938a0d30a91a3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -21,6 +21,8 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/Sequence.h"
+#include "llvm/ADT/SmallVector.h"
 #include <functional>
 #include <optional>
 
@@ -171,7 +173,7 @@ using FnCheck = std::function<void(const StreamChecker *, const FnDescription *,
                                    const CallEvent &, CheckerContext &)>;
 
 using ArgNoTy = unsigned int;
-static const ArgNoTy ArgNone = std::numeric_limits<ArgNoTy>::max();
+const ArgNoTy ArgNone = std::numeric_limits<ArgNoTy>::max();
 
 struct FnDescription {
   FnCheck PreFn;
@@ -179,6 +181,26 @@ struct FnDescription {
   ArgNoTy StreamArgNo;
 };
 
+[[nodiscard]] ProgramStateRef
+escapeArgsAfterIndex(ProgramStateRef State, CheckerContext &C,
+                     const CallEvent &Call, unsigned FirstEscapingArgIndex) {
+  const auto *CE = Call.getOriginExpr();
+  assert(CE);
+
+  if (Call.getNumArgs() <= FirstEscapingArgIndex)
+    return State;
+
+  SmallVector<SVal> EscapingArgs;
+  EscapingArgs.reserve(Call.getNumArgs() - FirstEscapingArgIndex);
+  for (auto EscArgIdx :
+       llvm::seq<int>(FirstEscapingArgIndex, Call.getNumArgs()))
+    EscapingArgs.push_back(Call.getArgSVal(EscArgIdx));
+  State = State->invalidateRegions(EscapingArgs, CE, C.blockCount(),
+                                   C.getLocationContext(),
+                                   /*CausesPointerEscape=*/false);
+  return State;
+}
+
 /// Get the value of the stream argument out of the passed call event.
 /// The call should contain a function that is described by Desc.
 SVal getStreamArg(const FnDescription *Desc, const CallEvent &Call) {
@@ -396,6 +418,18 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
         0}},
       {{{"fileno"}, 1},
        {&StreamChecker::preDefault, &StreamChecker::evalFileno, 0}},
+      {{{"getc"}, 1},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+        std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, true), 0}},
+      {{{"vfscanf"}, 3},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+        &StreamChecker::evalFscanf, 0}},
+      {{{"putc"}, 2},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
+        std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, true), 1}},
+      {{{"vfprintf"}, 3},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
+        &StreamChecker::evalFprintf, 0}},
   };
 
   CallDescriptionMap<FnDescription> FnTestDescriptions = {
@@ -997,6 +1031,9 @@ void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call,
   if (!E.Init(Desc, Call, C, State))
     return;
 
+  // The pointers passed to fscanf escape and get invalidated.
+  State = escapeArgsAfterIndex(State, C, Call, /*FirstEscapingArgIndex=*/2);
+
   // Add the success state.
   // In this context "success" means there is not an EOF or other read error
   // before any item is matched in 'fscanf'. But there may be match failure,
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..87688bd8b312f4 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,8 @@ int vprintf (const char *restrict format, va_list arg);
 
 int vsprintf (char *restrict s, const char *restrict format, va_list arg);
 
+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.c b/clang/test/Analysis/stream.c
index 378c9154f8f6a8..d0fee68d482e7f 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -1,8 +1,11 @@
 // RUN: %clang_analyze_cc1 -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 clang_analyzer_dump_char(char);
+void clang_analyzer_dump_int(int);
 
 void check_fread(void) {
   FILE *fp = tmpfile();
@@ -65,12 +68,24 @@ void check_fseek(void) {
   fclose(fp);
 }
 
+void check_fseeko(void) {
+  FILE *fp = tmpfile();
+  fseeko(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_ftello(void) {
+  FILE *fp = tmpfile();
+  ftello(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}}
@@ -138,6 +153,15 @@ void f_seek(void) {
   fclose(p);
 }
 
+void f_seeko(void) {
+  FILE *p = fopen("foo", "r");
+  if (!p)
+    return;
+  fseeko(p, 1, SEEK_SET); // no-warning
+  fseeko(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)
@@ -339,3 +363,107 @@ void fflush_on_open_failed_stream(void) {
   }
   fclose(F);
 }
+
+void test_fscanf_eof() {
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return;
+
+  int a;
+  unsigned b;
+  int ret = fscanf(F1, "%d %u", &a, &b);
+  char c = fgetc(F1); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
+  // expected-warning at -1 {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+  fclose(F1);
+}
+
+void test_fscanf_escape() {
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return;
+
+  int a = 48;
+  unsigned b = 127;
+  char buffer[] = "FSCANF"; // 70 83 67 65 78 70
+
+  clang_analyzer_dump_int(a); // expected-warning {{48 S32b}}
+  clang_analyzer_dump_int(b); // expected-warning {{127 S32b}}
+  clang_analyzer_dump_char(buffer[2]); // expected-warning {{67 S8b}}
+
+  int ret = fscanf(F1, "%d %u %s", &a, &b, buffer);
+  clang_analyzer_dump_int(a); // expected-warning {{conj_$}}
+  clang_analyzer_dump_int(b); // expected-warning {{conj_$}}
+  clang_analyzer_dump_char(buffer[2]); // expected-warning {{derived_$}}
+
+  if (ret != EOF) {
+    char c = fgetc(F1); // ok
+  }
+
+  fclose(F1);
+}
+
+void test_fputc() {
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return;
+
+  char a = 'y'; // 'y' = 121 ASCII
+  char r = fputc(a, F1);
+  if (r != EOF) {
+    clang_analyzer_dump_char(r); // expected-warning {{121 S8b}}
+    char z = fgetc(F1);
+  } else {
+    clang_analyzer_dump_char(r);  // expected-warning {{-1 S8b}}
+  }
+
+  fclose(F1);
+}
+
+void test_fputs() {
+  FILE *F1 = tmpfile();
+  if (!F1)
+    return;
+
+  char buffer[] = "HELLO";
+  int r = fputs(buffer, F1);
+  if (r >= 0) {
+    // fputs does not invalidate the input buffer (72 is ascii for 'H')
+    clang_analyzer_dump_char(buffer[0]); // expected-warning {{72 S8b}}
+  } else if (r == EOF) {
+    // fputs does not invalidate the input buffer, *and* this branch
+    // can happen
+    clang_analyzer_dump_char(buffer[0]); // expected-warning {{72 S8b}}
+  } else {
+    // This branch can not happen
+    int *p = NULL;
+    *p = 0;
+  }
+
+  fclose(F1);
+}
+
+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_int(a); // expected-warning {{42 S32b}}
+  clang_analyzer_dump_char(output[1]); // expected-warning {{69 S8b}}
+  if (r < 0) {
+    // Failure
+    fprintf(F1, "%s\t%u\n", output, a); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+  } else {
+    char buffer[10];
+    fscanf(F1, "%s", buffer);
+    if (fseek(F1, 0, SEEK_SET) == 0) {
+      fprintf(F1, "%s\t%u\n", buffer, a); // ok
+    }
+  }
+
+  fclose(F1);
+}

>From 4de634f0bd559085d50aa5f89afb79a246d1a03f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
 <alejandro.alvarez at sonarsource.com>
Date: Wed, 21 Feb 2024 14:05:39 +0100
Subject: [PATCH 02/10] Reintroduce static, rename escapeArgsAfterIndex

---
 clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 7938a0d30a91a3..44a62cf08c8862 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -173,7 +173,7 @@ using FnCheck = std::function<void(const StreamChecker *, const FnDescription *,
                                    const CallEvent &, CheckerContext &)>;
 
 using ArgNoTy = unsigned int;
-const ArgNoTy ArgNone = std::numeric_limits<ArgNoTy>::max();
+static const ArgNoTy ArgNone = std::numeric_limits<ArgNoTy>::max();
 
 struct FnDescription {
   FnCheck PreFn;
@@ -182,8 +182,9 @@ struct FnDescription {
 };
 
 [[nodiscard]] ProgramStateRef
-escapeArgsAfterIndex(ProgramStateRef State, CheckerContext &C,
-                     const CallEvent &Call, unsigned FirstEscapingArgIndex) {
+escapeArgsStartingFromIndex(ProgramStateRef State, CheckerContext &C,
+                            const CallEvent &Call,
+                            unsigned FirstEscapingArgIndex) {
   const auto *CE = Call.getOriginExpr();
   assert(CE);
 
@@ -1032,7 +1033,8 @@ void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call,
     return;
 
   // The pointers passed to fscanf escape and get invalidated.
-  State = escapeArgsAfterIndex(State, C, Call, /*FirstEscapingArgIndex=*/2);
+  State =
+      escapeArgsStartingFromIndex(State, C, Call, /*FirstEscapingArgIndex=*/2);
 
   // Add the success state.
   // In this context "success" means there is not an EOF or other read error

>From 3dc80ccaf2647d2d270e5b91f6a13538e68e2831 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
 <alejandro.alvarez at sonarsource.com>
Date: Wed, 21 Feb 2024 14:16:14 +0100
Subject: [PATCH 03/10] Reorder functions

---
 .../StaticAnalyzer/Checkers/StreamChecker.cpp | 24 +++++++++----------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 44a62cf08c8862..29ea88b77da120 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -370,18 +370,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,18 +431,6 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
         0}},
       {{{"fileno"}, 1},
        {&StreamChecker::preDefault, &StreamChecker::evalFileno, 0}},
-      {{{"getc"}, 1},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
-        std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, true), 0}},
-      {{{"vfscanf"}, 3},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
-        &StreamChecker::evalFscanf, 0}},
-      {{{"putc"}, 2},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
-        std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, true), 1}},
-      {{{"vfprintf"}, 3},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
-        &StreamChecker::evalFprintf, 0}},
   };
 
   CallDescriptionMap<FnDescription> FnTestDescriptions = {

>From c07738959a790515c2dcab1ff5a43ca7a1c6d8a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
 <alejandro.alvarez at sonarsource.com>
Date: Mon, 26 Feb 2024 13:38:06 +0100
Subject: [PATCH 04/10] Remove redundant invalidation in scanf

---
 .../StaticAnalyzer/Checkers/StreamChecker.cpp | 25 -------------------
 clang/test/Analysis/stream.c                  | 12 ++++++---
 2 files changed, 9 insertions(+), 28 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 0ef878df5f5104..8c8c7ff6fb832a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -180,27 +180,6 @@ struct FnDescription {
   ArgNoTy StreamArgNo;
 };
 
-[[nodiscard]] ProgramStateRef
-escapeArgsStartingFromIndex(ProgramStateRef State, CheckerContext &C,
-                            const CallEvent &Call,
-                            unsigned FirstEscapingArgIndex) {
-  const auto *CE = Call.getOriginExpr();
-  assert(CE);
-
-  if (Call.getNumArgs() <= FirstEscapingArgIndex)
-    return State;
-
-  SmallVector<SVal> EscapingArgs;
-  EscapingArgs.reserve(Call.getNumArgs() - FirstEscapingArgIndex);
-  for (auto EscArgIdx :
-       llvm::seq<int>(FirstEscapingArgIndex, Call.getNumArgs()))
-    EscapingArgs.push_back(Call.getArgSVal(EscArgIdx));
-  State = State->invalidateRegions(EscapingArgs, CE, C.blockCount(),
-                                   C.getLocationContext(),
-                                   /*CausesPointerEscape=*/false);
-  return State;
-}
-
 /// Get the value of the stream argument out of the passed call event.
 /// The call should contain a function that is described by Desc.
 SVal getStreamArg(const FnDescription *Desc, const CallEvent &Call) {
@@ -1054,10 +1033,6 @@ void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call,
   if (!E.Init(Desc, Call, C, State))
     return;
 
-  // The pointers passed to fscanf escape and get invalidated.
-  State =
-      escapeArgsStartingFromIndex(State, C, Call, /*FirstEscapingArgIndex=*/2);
-
   // Add the success state.
   // In this context "success" means there is not an EOF or other read error
   // before any item is matched in 'fscanf'. But there may be match failure,
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index d0fee68d482e7f..52520c967abefc 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -391,9 +391,15 @@ void test_fscanf_escape() {
   clang_analyzer_dump_char(buffer[2]); // expected-warning {{67 S8b}}
 
   int ret = fscanf(F1, "%d %u %s", &a, &b, buffer);
-  clang_analyzer_dump_int(a); // expected-warning {{conj_$}}
-  clang_analyzer_dump_int(b); // expected-warning {{conj_$}}
-  clang_analyzer_dump_char(buffer[2]); // expected-warning {{derived_$}}
+  if (ret != EOF) {
+    clang_analyzer_dump_int(a); // expected-warning {{conj_$}}
+    clang_analyzer_dump_int(b); // expected-warning {{conj_$}}
+    clang_analyzer_dump_char(buffer[2]); // expected-warning {{derived_$}}
+  } else {
+    clang_analyzer_dump_int(a); // expected-warning {{48 S32b}}
+    clang_analyzer_dump_int(b); // expected-warning {{127 S32b}}
+    clang_analyzer_dump_char(buffer[2]); // expected-warning {{67 S8b}}
+  }
 
   if (ret != EOF) {
     char c = fgetc(F1); // ok

>From 393cdf8ff273bd19aa99067423e84db0157ef765 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
 <alejandro.alvarez at sonarsource.com>
Date: Mon, 26 Feb 2024 15:04:33 +0100
Subject: [PATCH 05/10] vfscanf does not model escaping of parameters

---
 .../StaticAnalyzer/Checkers/StreamChecker.cpp | 11 +++++---
 clang/test/Analysis/stream.c                  | 25 +++++++++++++++++++
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 8c8c7ff6fb832a..29956fed2b3c24 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1050,10 +1050,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/stream.c b/clang/test/Analysis/stream.c
index 52520c967abefc..2688f658f61b4b 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -473,3 +473,28 @@ void test_fprintf() {
 
   fclose(F1);
 }
+
+
+int test_vscanf_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_vscanf() {
+  int i = 42;
+  int r = test_vscanf_inner("%d", &i);
+  if (r != EOF) {
+    clang_analyzer_dump_int(i); // expected-warning {{conj_$}}
+    // FIXME va_list "hides" the pointer to i
+  }
+}

>From dc1b34df1e7d4c8400825625d625f3a67754f697 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
 <alejandro.alvarez at sonarsource.com>
Date: Mon, 26 Feb 2024 17:17:51 +0100
Subject: [PATCH 06/10] Fix typo in test

---
 clang/test/Analysis/stream.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index 2688f658f61b4b..13bf685d2818eb 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -475,7 +475,7 @@ void test_fprintf() {
 }
 
 
-int test_vscanf_inner(const char *fmt, ...) {
+int test_vfscanf_inner(const char *fmt, ...) {
   FILE *F1 = tmpfile();
   if (!F1)
     return EOF;
@@ -490,9 +490,9 @@ int test_vscanf_inner(const char *fmt, ...) {
   return r;
 }
 
-void test_vscanf() {
+void test_vfscanf() {
   int i = 42;
-  int r = test_vscanf_inner("%d", &i);
+  int r = test_vfscanf_inner("%d", &i);
   if (r != EOF) {
     clang_analyzer_dump_int(i); // expected-warning {{conj_$}}
     // FIXME va_list "hides" the pointer to i

>From b493cc4573498ede3f22ad9ecfad27f5629befac Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
 <alejandro.alvarez at sonarsource.com>
Date: Mon, 26 Feb 2024 17:21:48 +0100
Subject: [PATCH 07/10] Remove redundant tests

---
 clang/test/Analysis/stream.c | 53 ------------------------------------
 1 file changed, 53 deletions(-)

diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index 13bf685d2818eb..022043cd21ac3f 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -364,19 +364,6 @@ void fflush_on_open_failed_stream(void) {
   fclose(F);
 }
 
-void test_fscanf_eof() {
-  FILE *F1 = tmpfile();
-  if (!F1)
-    return;
-
-  int a;
-  unsigned b;
-  int ret = fscanf(F1, "%d %u", &a, &b);
-  char c = fgetc(F1); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
-  // expected-warning at -1 {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
-  fclose(F1);
-}
-
 void test_fscanf_escape() {
   FILE *F1 = tmpfile();
   if (!F1)
@@ -408,46 +395,6 @@ void test_fscanf_escape() {
   fclose(F1);
 }
 
-void test_fputc() {
-  FILE *F1 = tmpfile();
-  if (!F1)
-    return;
-
-  char a = 'y'; // 'y' = 121 ASCII
-  char r = fputc(a, F1);
-  if (r != EOF) {
-    clang_analyzer_dump_char(r); // expected-warning {{121 S8b}}
-    char z = fgetc(F1);
-  } else {
-    clang_analyzer_dump_char(r);  // expected-warning {{-1 S8b}}
-  }
-
-  fclose(F1);
-}
-
-void test_fputs() {
-  FILE *F1 = tmpfile();
-  if (!F1)
-    return;
-
-  char buffer[] = "HELLO";
-  int r = fputs(buffer, F1);
-  if (r >= 0) {
-    // fputs does not invalidate the input buffer (72 is ascii for 'H')
-    clang_analyzer_dump_char(buffer[0]); // expected-warning {{72 S8b}}
-  } else if (r == EOF) {
-    // fputs does not invalidate the input buffer, *and* this branch
-    // can happen
-    clang_analyzer_dump_char(buffer[0]); // expected-warning {{72 S8b}}
-  } else {
-    // This branch can not happen
-    int *p = NULL;
-    *p = 0;
-  }
-
-  fclose(F1);
-}
-
 void test_fprintf() {
   FILE *F1 = tmpfile();
   if (!F1)

>From 4b23bca61cdbe5bf1b48c000b2746efd725d7e03 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
 <alejandro.alvarez at sonarsource.com>
Date: Mon, 26 Feb 2024 17:29:06 +0100
Subject: [PATCH 08/10] Move invalidation tests to stream-invalidate.c

---
 clang/test/Analysis/stream-invalidate.c | 51 +++++++++++++++
 clang/test/Analysis/stream.c            | 85 -------------------------
 2 files changed, 51 insertions(+), 85 deletions(-)

diff --git a/clang/test/Analysis/stream-invalidate.c b/clang/test/Analysis/stream-invalidate.c
index 6745d11a2fe701..2746edfa33f2da 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,53 @@ 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}}
+  if (r < 0) {
+    // Failure
+    fprintf(F1, "%s\t%u\n", output, a); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+  } else {
+    char buffer[10];
+    fscanf(F1, "%s", buffer);
+    if (fseek(F1, 0, SEEK_SET) == 0) {
+      fprintf(F1, "%s\t%u\n", buffer, a); // ok
+    }
+  }
+
+  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 r = test_vfscanf_inner("%d", &i);
+  if (r != EOF) {
+    clang_analyzer_dump(i); // expected-warning {{conj_$}}
+    // FIXME va_list "hides" the pointer to i
+  }
+}
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index 022043cd21ac3f..186fd84154ba72 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -1,11 +1,8 @@
 // RUN: %clang_analyze_cc1 -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 clang_analyzer_dump_char(char);
-void clang_analyzer_dump_int(int);
 
 void check_fread(void) {
   FILE *fp = tmpfile();
@@ -363,85 +360,3 @@ void fflush_on_open_failed_stream(void) {
   }
   fclose(F);
 }
-
-void test_fscanf_escape() {
-  FILE *F1 = tmpfile();
-  if (!F1)
-    return;
-
-  int a = 48;
-  unsigned b = 127;
-  char buffer[] = "FSCANF"; // 70 83 67 65 78 70
-
-  clang_analyzer_dump_int(a); // expected-warning {{48 S32b}}
-  clang_analyzer_dump_int(b); // expected-warning {{127 S32b}}
-  clang_analyzer_dump_char(buffer[2]); // expected-warning {{67 S8b}}
-
-  int ret = fscanf(F1, "%d %u %s", &a, &b, buffer);
-  if (ret != EOF) {
-    clang_analyzer_dump_int(a); // expected-warning {{conj_$}}
-    clang_analyzer_dump_int(b); // expected-warning {{conj_$}}
-    clang_analyzer_dump_char(buffer[2]); // expected-warning {{derived_$}}
-  } else {
-    clang_analyzer_dump_int(a); // expected-warning {{48 S32b}}
-    clang_analyzer_dump_int(b); // expected-warning {{127 S32b}}
-    clang_analyzer_dump_char(buffer[2]); // expected-warning {{67 S8b}}
-  }
-
-  if (ret != EOF) {
-    char c = fgetc(F1); // ok
-  }
-
-  fclose(F1);
-}
-
-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_int(a); // expected-warning {{42 S32b}}
-  clang_analyzer_dump_char(output[1]); // expected-warning {{69 S8b}}
-  if (r < 0) {
-    // Failure
-    fprintf(F1, "%s\t%u\n", output, a); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
-  } else {
-    char buffer[10];
-    fscanf(F1, "%s", buffer);
-    if (fseek(F1, 0, SEEK_SET) == 0) {
-      fprintf(F1, "%s\t%u\n", buffer, a); // ok
-    }
-  }
-
-  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 r = test_vfscanf_inner("%d", &i);
-  if (r != EOF) {
-    clang_analyzer_dump_int(i); // expected-warning {{conj_$}}
-    // FIXME va_list "hides" the pointer to i
-  }
-}

>From e33b7f20f41a21a334d1a5e83a16822fe063d700 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
 <alejandro.alvarez at sonarsource.com>
Date: Mon, 26 Feb 2024 17:30:50 +0100
Subject: [PATCH 09/10] Remove test code that does not test for invalidation

---
 clang/test/Analysis/stream-invalidate.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/clang/test/Analysis/stream-invalidate.c b/clang/test/Analysis/stream-invalidate.c
index 2746edfa33f2da..c28dcfe5a2825e 100644
--- a/clang/test/Analysis/stream-invalidate.c
+++ b/clang/test/Analysis/stream-invalidate.c
@@ -159,17 +159,6 @@ void test_fprintf() {
   // 69 is ascii for 'E'
   clang_analyzer_dump(a); // expected-warning {{42 S32b}}
   clang_analyzer_dump(output[1]); // expected-warning {{69 S32b}}
-  if (r < 0) {
-    // Failure
-    fprintf(F1, "%s\t%u\n", output, a); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
-  } else {
-    char buffer[10];
-    fscanf(F1, "%s", buffer);
-    if (fseek(F1, 0, SEEK_SET) == 0) {
-      fprintf(F1, "%s\t%u\n", buffer, a); // ok
-    }
-  }
-
   fclose(F1);
 }
 

>From cf41e836adafea662d5acd146cf4fd8573fd31ab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?=
 <alejandro.alvarez at sonarsource.com>
Date: Mon, 26 Feb 2024 17:36:30 +0100
Subject: [PATCH 10/10] More precise explanation for test_vfscanf

---
 clang/test/Analysis/stream-invalidate.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/clang/test/Analysis/stream-invalidate.c b/clang/test/Analysis/stream-invalidate.c
index c28dcfe5a2825e..5046a356d0583d 100644
--- a/clang/test/Analysis/stream-invalidate.c
+++ b/clang/test/Analysis/stream-invalidate.c
@@ -179,9 +179,11 @@ int test_vfscanf_inner(const char *fmt, ...) {
 
 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_$}}
-    // FIXME va_list "hides" the pointer to i
+    clang_analyzer_dump(j); // expected-warning {{43 S32b}}
   }
 }



More information about the cfe-commits mailing list