Skip to content

Commit

Permalink
Reapply "[clang][analyzer] StreamChecker: Model getc, vfscanf, putc, …
Browse files Browse the repository at this point in the history
…vfprintf" (#83281)

`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.
  • Loading branch information
alejandro-alvarez-sonarsource committed Mar 6, 2024
1 parent e854702 commit 239312e
Show file tree
Hide file tree
Showing 6 changed files with 118 additions and 7 deletions.
33 changes: 28 additions & 5 deletions clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -318,18 +318,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}},
Expand Down Expand Up @@ -389,6 +401,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;
Expand Down Expand Up @@ -518,7 +532,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;
}

Expand Down Expand Up @@ -557,6 +572,10 @@ 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,
Expand Down Expand Up @@ -705,6 +724,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)
Expand Down Expand Up @@ -1085,10 +1105,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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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" );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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.
Expand Down
3 changes: 3 additions & 0 deletions clang/test/Analysis/Inputs/system-header-simulator.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
42 changes: 42 additions & 0 deletions clang/test/Analysis/stream-invalidate.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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}}
}
}
39 changes: 38 additions & 1 deletion clang/test/Analysis/stream.c
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
// 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);

Expand Down Expand Up @@ -65,12 +69,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}}
Expand Down Expand Up @@ -129,6 +145,18 @@ void f_dopen(int fd) {
fclose(F);
}

void f_vfprintf(int fd, va_list args) {
FILE *F = fdopen(fd, "r");
vfprintf(F, "%d", args); // expected-warning {{Stream pointer might be NULL}}
fclose(F);
}

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

void f_seek(void) {
FILE *p = fopen("foo", "r");
if (!p)
Expand All @@ -138,6 +166,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)
Expand Down

0 comments on commit 239312e

Please sign in to comment.