Reapply "[clang][analyzer] StreamChecker: Model getc, vfscanf, putc, … #83281
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Alejandro Álvarez Ayllón (alejandro-alvarez-sonarsource) Changes…vfprintf (#82476)"
This reverts commit 570bc5d. Full diff: https://github.com/llvm/llvm-project/pull/83281.diff 6 Files Affected:
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);
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…vfprintf (llvm#82476)" This reverts commit 570bc5d
alejandro-alvarez-sonarsource
force-pushed
the
aa/stream_c_apis
branch
from
ddbe895 to
a9419bc
Compare
|
@balazske @steakhal I broke some tests on aarch64 since there Not sure if that is the best way, though. |
|
@alejandro-alvarez-sonarsource Should we merge this, and give it a try? |
|
Well, the test now runs for 4 different platforms and the PR checks are all green, so I guess we can merge. |
steakhal
approved these changes
Mar 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…vfprintf (#82476)"
va_listis a platform-specific type. On some, it is a struct instead of a pointer to a struct, solookupFnwas ignoring calls tovfprintfandvfscanf.stream.cnow runs in four different platforms to make sure the logic works across targets.This reverts commit 570bc5d.