Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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}},
Expand Down Expand Up @@ -1038,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);
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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks not needed.

Copy link
Contributor Author

@alejandro-alvarez-sonarsource alejandro-alvarez-sonarsource Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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}}
}
}
34 changes: 34 additions & 0 deletions clang/test/Analysis/stream.c
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// 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);

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

void check_fseeko(void) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

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 +142,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 +163,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