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

alejandro-alvarez-sonarsource · 2024-02-21T09:53:48Z

Model getc and putc as equivalent to fgetc and fputc respectively.

Model vfscanf and vfprintf as fscanf and fprintf, except that vfscanf can not invalidate the parameters due to the indirection via a va_list. Nevertheless, we can still track EOF and errors as for fscanf.

llvmbot · 2024-02-21T09:54:20Z

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

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

Changes

Model getc, vfscanf, putc, vfprintf.
fscanf invalidates all arguments after the format string.

Also, add tests for ftello and fseeko.

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

5 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+38-1)
(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 (+4)
(modified) clang/test/Analysis/Inputs/system-header-simulator.h (+3)
(modified) clang/test/Analysis/stream.c (+128)

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@-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);
+}

llvmbot · 2024-02-21T09:54:20Z

@llvm/pr-subscribers-clang

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

Changes

Model getc, vfscanf, putc, vfprintf.
fscanf invalidates all arguments after the format string.

Also, add tests for ftello and fseeko.

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

5 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+38-1)
(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 (+4)
(modified) clang/test/Analysis/Inputs/system-header-simulator.h (+3)
(modified) clang/test/Analysis/stream.c (+128)

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@-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);
+}

balazske · 2024-02-21T11:20:54Z

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

@@ -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}},


Entry for getc belongs (directly) after fgets (or fgetc).

balazske

How does this relate to the other PR #79470 (why comes this new patch when there is another for the same problem)?
I think it is better to first commit #79470, then add remaining functions getc, putc, vfscanf, vfprintf. These should be added anyway in a separate change because it is less related to invalidation.

balazske · 2024-02-21T11:21:46Z

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

+        &StreamChecker::evalFscanf, 0}},
+      {{{"putc"}, 2},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
+        std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, true), 1}},


Entry for putc belongs (directly) after fputs (or fputc).

NagyDonat

Looks good overall, I only have a few small suggestions/questions.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

clang/test/Analysis/stream.c

alejandro-alvarez-sonarsource · 2024-02-21T13:12:27Z

How does this relate to the other PR #79470 (why comes this new patch when there is another for the same problem)? I think it is better to first commit #79470, then add remaining functions getc, putc, vfscanf, vfprintf. These should be added anyway in a separate change because it is less related to invalidation.

I started the work on this patch before #79470 was submitted, and didn't double-check if someone had tackled the same in the meantime. My bad.

I agree on waiting for that one to land and strip this one down to the new set of functions.

alejandro-alvarez-sonarsource · 2024-02-26T15:23:14Z

I have reduced the scope of the PR, and changed the title and the description accordingly.

balazske

I would like better if the new tests that are invalidation-related are moved to file stream-invalidate.c (this was added in the recent commit).

balazske · 2024-02-26T15:58:20Z

clang/test/Analysis/stream.c

@@ -339,3 +363,138 @@ void fflush_on_open_failed_stream(void) {
  }
  fclose(F);
 }
+
+void test_fscanf_eof() {


Has this test a purpose that is different from test error_fscanf in stream_error.c?

balazske · 2024-02-26T16:01:34Z

clang/test/Analysis/stream.c

+  fclose(F1);
+}
+
+void test_fputc() {


This test looks similar (in purpose) to error_fputc (stream_error.c) too.

Removed since it is redundant.

balazske · 2024-02-26T16:04:11Z

clang/test/Analysis/stream.c

+    // can happen
+    clang_analyzer_dump_char(buffer[0]); // expected-warning {{72 S8b}}
+  } else {
+    // This branch can not happen


clang_analyzer_warnIfReached can be used here, but really this case is tested in another test in stream_error.c too.

Likewise, removed.

balazske · 2024-02-26T16:07:15Z

clang/test/Analysis/stream.c

+}
+
+
+int test_vscanf_inner(const char *fmt, ...) {


Should the test name contain vfscanf?

Indeed, sorry for the typo.

balazske · 2024-02-26T16:08:02Z

clang/test/Analysis/stream.c

+  return r;
+}
+
+void test_vscanf() {


Here the test name looks incorrect too, and the called function in the test.

Same typo, fixed.

balazske

I have only small observations.

balazske · 2024-02-26T17:07:40Z

clang/test/Analysis/Inputs/system-header-simulator-for-simple-stream.h

@@ -5,7 +5,7 @@
 // suppressed.
 #pragma clang system_header

-typedef struct __sFILE {


This change looks not needed.

I had to modify this line after adding vfscanf (and now vfprintf) to system-header-simulator-for-valist.h
The reason is that they need the FILE* typedef and they have to be consistent with the typedef of system-header-simulator.h since they are used together.

But system-header-simulator-for-valist.h was already used together with system-header-simulator-for-simple-stream.h in security-syntax-checks.m.

So I had to change something to keep them consistent, and typedef struct _FILE FILE; seems to be used more commonly over the tests, and less likely to cause conflicts in the future.

balazske · 2024-02-26T17:15:01Z

clang/test/Analysis/stream.c

@@ -65,12 +65,24 @@ void check_fseek(void) {
  fclose(fp);
 }

+void check_fseeko(void) {


If we have these null pointer tests for all functions, vfprintf and vfscanf could be added too.

alejandro-alvarez-sonarsource · 2024-02-28T10:03:25Z

Rebased on top of main to solve a conflict.

1. Model getc, vfscanf, putc, vfprintf. 2. fscanf invalidates all arguments after the format string.

…fprintf (#82476)" This reverts commit ffe7049. This commit breaks on e.g. arm: Example: https://lab.llvm.org/buildbot/#/builders/245/builds/21177/steps/5/logs/FAIL__Clang__stream_c ``` ******************** TEST 'Clang :: Analysis/stream.c' FAILED ******************** Exit Code: 1 Command Output (stderr): -- RUN: at line 1: /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/clang -cc1 -internal-isystem /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/lib/clang/19/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/Analysis/stream.c + /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/clang -cc1 -internal-isystem /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/lib/clang/19/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/Analysis/stream.c error: 'expected-warning' diagnostics expected but not seen: File /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/Analysis/stream.c Line 147: Stream pointer might be NULL File /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/Analysis/stream.c Line 153: Stream pointer might be NULL error: 'expected-warning' diagnostics seen but not expected: File /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/Analysis/stream.c Line 148: Stream pointer might be NULL [alpha.unix.Stream] File /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/Analysis/stream.c Line 154: Stream pointer might be NULL [alpha.unix.Stream] 4 errors generated. -- ******************** ```

…vfprintf (llvm#82476)" This reverts commit 570bc5d

balazske · 2024-03-01T17:06:41Z

Do you plan to commit this re-apply to the LLVM repository?

alejandro-alvarez-sonarsource changed the title ~~[clang][analyzer] StreamChecker: add more APIs, invalidate fscanf args~~ [clang][analyzer] StreamChecker: Add more APIs, invalidate fscanf args Feb 21, 2024

llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Feb 21, 2024

steakhal requested review from balazske, NagyDonat and benshi001 and removed request for balazske and NagyDonat February 21, 2024 10:11

balazske reviewed Feb 21, 2024

View reviewed changes

NagyDonat reviewed Feb 21, 2024

View reviewed changes

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

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

clang/test/Analysis/stream.c Outdated Show resolved Hide resolved

alejandro-alvarez-sonarsource changed the title ~~[clang][analyzer] StreamChecker: Add more APIs, invalidate fscanf args~~ [clang][analyzer] StreamChecker: Model getc, vfscanf, putc, vfprintf Feb 26, 2024

alejandro-alvarez-sonarsource force-pushed the aa/streamchecker branch from a82f5f8 to 393cdf8 Compare February 26, 2024 14:09

alejandro-alvarez-sonarsource requested review from balazske and NagyDonat February 26, 2024 15:22

balazske requested changes Feb 26, 2024

View reviewed changes

balazske reviewed Feb 26, 2024

View reviewed changes

balazske approved these changes Feb 27, 2024

View reviewed changes

alejandro-alvarez-sonarsource force-pushed the aa/streamchecker branch from 02ca6fa to db12a09 Compare February 28, 2024 10:02

alejandro-alvarez-sonarsource added 4 commits February 28, 2024 12:22

[clang][analyzer] StreamChecker: add more APIs, invalidate fscanf args

414fb84

1. Model getc, vfscanf, putc, vfprintf. 2. fscanf invalidates all arguments after the format string.

Reintroduce static, rename escapeArgsAfterIndex

9d8f44d

Reorder functions

aa34150

Remove redundant invalidation in scanf

71a9fe0

alejandro-alvarez-sonarsource added 7 commits February 28, 2024 12:22

vfscanf does not model escaping of parameters

62c315d

Fix typo in test

9b1e3c9

Remove redundant tests

8ccd54e

Move invalidation tests to stream-invalidate.c

6893893

Remove test code that does not test for invalidation

14af3ab

More precise explanation for test_vfscanf

c8c9bd3

Add NULL fd test for vfscanf and vfprintf

2f6c479

steakhal force-pushed the aa/streamchecker branch from db12a09 to 2f6c479 Compare February 28, 2024 11:22

steakhal merged commit ffe7049 into llvm:main Feb 28, 2024

alejandro-alvarez-sonarsource mentioned this pull request Feb 28, 2024

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

Merged

alejandro-alvarez-sonarsource added a commit to alejandro-alvarez-sonarsource/llvm-project that referenced this pull request Feb 28, 2024

Reapply "[clang][analyzer] StreamChecker: Model getc, vfscanf, putc, …

a9419bc

…vfprintf (llvm#82476)" This reverts commit 570bc5d

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

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

Uh oh!

Conversation

alejandro-alvarez-sonarsource commented Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 21, 2024

Uh oh!

llvmbot commented Feb 21, 2024

Uh oh!

balazske Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balazske left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NagyDonat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alejandro-alvarez-sonarsource commented Feb 21, 2024

Uh oh!

alejandro-alvarez-sonarsource commented Feb 26, 2024

Uh oh!

balazske left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balazske left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alejandro-alvarez-sonarsource Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alejandro-alvarez-sonarsource commented Feb 28, 2024

Uh oh!

balazske commented Mar 1, 2024

Uh oh!

Uh oh!

alejandro-alvarez-sonarsource commented Feb 21, 2024 •

edited

Loading

balazske Feb 21, 2024 •

edited

Loading

alejandro-alvarez-sonarsource Feb 27, 2024 •

edited

Loading