Skip to content

[clang][analyzer] Add C standard streams to the internal memory space #147766

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+61 −6
Open

[clang][analyzer] Add C standard streams to the internal memory space #147766

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8dc303
[clang][analyzer] Add C standard streams to the internal memory space
balazske Jul 9, 2025
3117a64
updated code comment
balazske Jul 10, 2025
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
20 changes: 17 additions & 3 deletions clang/lib/StaticAnalyzer/Core/MemRegion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,10 +1054,24 @@ const VarRegion *MemRegionManager::getVarRegion(const VarDecl *D,
assert(!Ty.isNull());
if (Ty.isConstQualified()) {
sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
} else if (Ctx.getSourceManager().isInSystemHeader(D->getLocation())) {
sReg = getGlobalsRegion(MemRegion::GlobalSystemSpaceRegionKind);
} else {
sReg = getGlobalsRegion(MemRegion::GlobalInternalSpaceRegionKind);
StringRef N = D->getNameAsString();
Copy link
Contributor

Choose a reason for hiding this comment

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

getNameAsString() returns a std::string prvalue, thus we bind a view to this temporary that gets immediately dangling after the full-expression.

QualType FILETy = D->getASTContext().getFILEType();
if (!FILETy.isNull())
FILETy = FILETy.getCanonicalType();
Ty = Ty.getCanonicalType();
bool IsStdStreamVar = Ty->isPointerType() &&
Ty->getPointeeType() == FILETy &&
(N == "stdin" || N == "stdout" || N == "stderr");
Comment on lines +1059 to +1065
Copy link
Contributor

Choose a reason for hiding this comment

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

All this work is basically done to set the IsStdStreamVar.
This seems pretty complicated and not strictly necessary for getVarRegion itself.
This prompts me if we should hoist this into a helper function that we can call here. WDYT?

// Pointer value of C standard streams is usually not modified by system
// calls. This means they should not get invalidated at system calls and
// can not belong to the system memory space.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Pointer value of C standard streams is usually not modified by system
// calls. This means they should not get invalidated at system calls and
// can not belong to the system memory space.
// Pointer value of C standard streams is usually not modified by calls
// to functions declared in system headers. This means that they should
// not get invalidated by calls to functions declared in system headers,
// so they are placed in the global internal space, which is not
// invalidated by calls to functions declared in system headers.

I would prefer avoiding the phrase "system call" because it has a very specific meaning which is not what you speak about here.

Also, I extended the comment to describe the reason why is the global internal space a "better" place for these streams, because I felt that this makes it easier to quickly understand the goals of this logic.

if (Ctx.getSourceManager().isInSystemHeader(D->getLocation()) &&
!IsStdStreamVar) {
sReg = getGlobalsRegion(MemRegion::GlobalSystemSpaceRegionKind);
} else {
sReg = getGlobalsRegion(MemRegion::GlobalInternalSpaceRegionKind);
}
}

// Finally handle static locals.
Expand Down
45 changes: 42 additions & 3 deletions clang/test/Analysis/stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,14 +519,53 @@ void reopen_std_stream(void) {
if (!fp) return;

stdout = fp; // Let's make them alias.
clang_analyzer_eval(fp == oldStdout); // expected-warning {{UNKNOWN}}
clang_analyzer_eval(fp == stdout); // expected-warning {{TRUE}} no-FALSE
clang_analyzer_eval(oldStdout == stdout); // expected-warning {{UNKNOWN}}
clang_analyzer_eval(fp == oldStdout); // expected-warning {{FALSE}}
clang_analyzer_eval(fp == stdout); // expected-warning {{TRUE}}
clang_analyzer_eval(oldStdout == stdout); // expected-warning {{FALSE}}
}

void only_success_path_does_not_alias_with_stdout(void) {
if (stdout) return;
FILE *f = fopen("/tmp/foof", "r"); // no-crash
clang_analyzer_eval(f == 0);// expected-warning {{TRUE}} expected-warning {{FALSE}}
if (!f) return;
fclose(f);
}

extern void do_something();

void test_no_invalidate_at_system_call(int use_std) {
FILE *fd;
char *buf;

if (use_std) {
fd = stdin;
} else {
if ((fd = fopen("x/y/z", "r")) == NULL)
return;

clang_analyzer_eval(fd == stdin); // expected-warning{{FALSE}}
buf = (char *)malloc(100);
clang_analyzer_eval(fd == stdin); // expected-warning{{FALSE}}
}

if (fd != stdin)
fclose(fd);
}
Comment on lines +537 to +554
Copy link
Contributor

Choose a reason for hiding this comment

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

If I read this correctly, the test intends to check if the stdin system global would not get invalidated after calling malloc() (or for that matter any other opaque system header fn), right?

Could we simplify this into:

auto orig_stdin = stdin;
free(malloc(10)); // calling some opaque system fns
clang_analyzer_eval(stdin == orig_stdin); // TRUE

Same suggestion goes to the next example too.


void test_invalidate_at_non_system_call(int use_std) {
FILE *fd;

if (use_std) {
fd = stdin;
} else {
if ((fd = fopen("x/y/z", "r")) == NULL)
return;
}

do_something();
clang_analyzer_eval(fd == stdin); // expected-warning{{UNKNOWN}}

if (fd != stdin)
fclose(fd);
} // expected-warning{{Opened stream never closed. Potential resource leak}}