On Wed, 2026-03-25 at 13:07 +0100, Tomás Ortín Fernández wrote: > Add known_function handlers for fdopen and freopen. Both check that > the mode string is null-terminated, and both participate in the > FILE-pointer state machine so the analyzer can detect leaks and > double-fclose. > > For freopen, the handler returns the stream argument (since freopen > returns the same stream pointer on success). This is an initial > implementation: success/failure bifurcation is not yet implemented, > so > the failure path where the original stream is closed cannot be > modeled > without false positives. > > Also move fopen registration from the POSIX section to the C standard > section, and fix minor formatting issues in kf_fopen. > > Bootstrapped and tested on x86_64-pc-linux-gnu.
Hi Tomás, thanks for the patch. Various comments inline below throughout. Overall I think the patch is promising, but there are a bunch of TODOs that really need addressing. Would it help to split these into two separate patches? (I don't know, but I thought it worth mentioning) My gut feeling is that freopen is somewhat easier that fdopen, since freopen only has to interact with sm-file.cc, whereas fdopen probably needs to interact with both sm- file.cc and sm-fd.cc. > > gcc/analyzer/ChangeLog: > > * kf.cc (class kf_fopen): Fix formatting. The changes do rather more than just that; they add a check for "mode" being null-terminated. > (class kf_fdopen): New known_function handler. > (class kf_freopen): New known_function handler. > (register_known_functions): Register kf_fdopen and kf_freopen. > Move fopen from POSIX to C standard section. > * sm-file.cc (fileptr_state_machine::on_stmt): Handle fdopen > and freopen in the FILE-pointer state machine. > > gcc/testsuite/ChangeLog: > > * gcc.dg/analyzer/fopen-1.c: Add test_null_mode. > * gcc.dg/analyzer/fdopen-1.c: New test. > * gcc.dg/analyzer/freopen-1.c: New test. > > Signed-off-by: Tomas Ortin Fernandez (quanrong) <[email protected]> > --- > gcc/analyzer/kf.cc | 78 ++++++++++++++- > gcc/analyzer/sm-file.cc | 4 +- > gcc/testsuite/gcc.dg/analyzer/fdopen-1.c | 57 +++++++++++ > gcc/testsuite/gcc.dg/analyzer/fopen-1.c | 8 ++ > gcc/testsuite/gcc.dg/analyzer/freopen-1.c | 114 ++++++++++++++++++++++ > 5 files changed, 255 insertions(+), 6 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/analyzer/fdopen-1.c > create mode 100644 gcc/testsuite/gcc.dg/analyzer/freopen-1.c > > diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc > index 0434ab0f0bf..63365f2ef91 100644 > --- a/gcc/analyzer/kf.cc > +++ b/gcc/analyzer/kf.cc > @@ -530,16 +530,19 @@ kf_error::impl_call_pre (const call_details &cd) const > class kf_fopen : public known_function > { > public: > - bool matches_call_types_p (const call_details &cd) const final override > + bool > + matches_call_types_p (const call_details &cd) const final override > { > - return (cd.num_args () == 2 > - && cd.arg_is_pointer_p (0) > + return (cd.num_args () == 2 && cd.arg_is_pointer_p (0) > && cd.arg_is_pointer_p (1)); > } > > - void impl_call_pre (const call_details &cd) const final override > + void > + impl_call_pre (const call_details &cd) const final override > { > cd.check_for_null_terminated_string_arg (0); > + /* TODO: mode must not be NULL, but > check_for_null_terminated_string_arg > + doesn't yet warn on NULL pointers. */ > cd.check_for_null_terminated_string_arg (1); > cd.set_any_lhs_with_defaults (); > > @@ -548,6 +551,69 @@ public: > } > }; > > +/* Handler for "fdopen": > + FILE *fdopen (int fd, const char *mode); */ > + > +class kf_fdopen : public known_function > +{ > +public: > + bool > + matches_call_types_p (const call_details &cd) const final override > + { > + return (cd.num_args () == 2 && cd.arg_is_integral_p (0) > + && cd.arg_is_pointer_p (1)); > + } > + > + void > + impl_call_pre (const call_details &cd) const final override > + { > + /* TODO: check that fd is valid: warn on negative constants and > + integrate with the fd state machine to detect use of closed or > + invalid file descriptors. */ Should there be an interaction between the fd and file state machines? This is a big can of worms: we need to at least make sure we don't get false positives about leaks from the fd state machine, and to ensure we do get true positives about leaks from the file state machine. Consider something like: int fd = open (pathname, flags); if (fd == -1) return; FILE *fp = fdopen (fd, "r"); // shouldn't complain about leak of 'fd' return; // should complain about leak of 'fp'. In theory we could complain about mismatches between the mode of the fd and the mode of the fdopen, but IIRC there a bunch of platform-specific extensions to the mode string codes and we probably don't want to get into dealing with that. > + /* TODO: mode must not be NULL, but > check_for_null_terminated_string_arg > + doesn't yet warn on NULL pointers. */ > + cd.check_for_null_terminated_string_arg (1); > + cd.set_any_lhs_with_defaults (); > + } > +}; > + > +/* Handler for "freopen": > + FILE *freopen (const char *path, const char *mode, FILE *stream); > + > + The path argument can be NULL (reopen same file with new mode). > + > + On success freopen returns stream. On failure it returns NULL and > + the original stream is closed. Without success/failure bifurcation > + we cannot distinguish these paths, so we do not transition stream > + to "closed" in the state machine (doing so would cause false > + positives on the success path where stream is still usable). > + > + TODO: add success/failure bifurcation so that the failure path can > + transition stream to "closed." */ I think we probably need that bifurcation. > + > +class kf_freopen : public known_function > +{ > +public: > + bool > + matches_call_types_p (const call_details &cd) const final override > + { > + return (cd.num_args () == 3 && cd.arg_is_pointer_p (0) > + && cd.arg_is_pointer_p (1) && cd.arg_is_pointer_p (2)); > + } > + > + void > + impl_call_pre (const call_details &cd) const final override > + { > + /* path (arg 0) may be NULL (reopen same file with new mode). */ > + if (!zerop (cd.get_arg_tree (0))) > + cd.check_for_null_terminated_string_arg (0); > + /* TODO: mode must not be NULL, but > check_for_null_terminated_string_arg > + doesn't yet warn on NULL pointers. */ > + cd.check_for_null_terminated_string_arg (1); > + cd.maybe_set_lhs (cd.get_arg_svalue (2)); > + } > +}; > + > /* Handler for "free", after sm-handling. > > If the ptr points to an underlying heap region, delete the region, > @@ -2942,8 +3008,10 @@ register_known_functions (known_function_manager > &kfm, > kfm.add ("__builtin_alloca", std::make_unique<kf_alloca> ()); > kfm.add ("calloc", std::make_unique<kf_calloc> ()); > kfm.add ("__builtin_calloc", std::make_unique<kf_calloc> ()); > + kfm.add ("fopen", std::make_unique<kf_fopen> ()); > kfm.add ("free", std::make_unique<kf_free> ()); > kfm.add ("__builtin_free", std::make_unique<kf_free> ()); > + kfm.add ("freopen", std::make_unique<kf_freopen> ()); > kfm.add ("malloc", std::make_unique<kf_malloc> ()); > kfm.add ("__builtin_malloc", std::make_unique<kf_malloc> ()); > kfm.add ("memcpy", > @@ -2994,7 +3062,7 @@ register_known_functions (known_function_manager &kfm, > > /* Known POSIX functions, and some non-standard extensions. */ > { > - kfm.add ("fopen", std::make_unique<kf_fopen> ()); > + kfm.add ("fdopen", std::make_unique<kf_fdopen> ()); > kfm.add ("mkdtemp", std::make_unique<kf_mktemp_simple> ( > kf_mktemp_family::outcome::null_ptr)); > kfm.add ("mkostemp", std::make_unique<kf_mkostemp> ()); > diff --git a/gcc/analyzer/sm-file.cc b/gcc/analyzer/sm-file.cc > index b8a0e2bda78..980ac005610 100644 > --- a/gcc/analyzer/sm-file.cc > +++ b/gcc/analyzer/sm-file.cc > @@ -410,7 +410,9 @@ fileptr_state_machine::on_stmt (sm_context &sm_ctxt, > if (const gcall *call = dyn_cast <const gcall *> (stmt)) > if (tree callee_fndecl = sm_ctxt.get_fndecl_for_call (*call)) > { > - if (is_named_call_p (callee_fndecl, "fopen", *call, 2)) > + if (is_named_call_p (callee_fndecl, "fopen", *call, 2) > + || is_named_call_p (callee_fndecl, "fdopen", *call, 2) > + || is_named_call_p (callee_fndecl, "freopen", *call, 3)) > { > tree lhs = gimple_call_lhs (call); > if (lhs) > diff --git a/gcc/testsuite/gcc.dg/analyzer/fdopen-1.c > b/gcc/testsuite/gcc.dg/analyzer/fdopen-1.c > new file mode 100644 > index 00000000000..e4213ba3d99 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/analyzer/fdopen-1.c > @@ -0,0 +1,57 @@ > +/* { dg-prune-output "-Wuse-after-free" } */ > + > +#include "analyzer-decls.h" > + > +typedef struct FILE FILE; > +FILE *fdopen (int fd, const char *mode); > +int fclose (FILE *); > + > +FILE * > +test_passthrough (int fd, const char *mode) > +{ > + return fdopen (fd, mode); > +} > + > +FILE * > +test_null_mode (int fd) > +{ > + return fdopen (fd, NULL); /* TODO: should warn about NULL mode. */ > +} > + > +FILE * > +test_simple_r (int fd) > +{ > + return fdopen (fd, "r"); > +} > + > +FILE * > +test_unterminated_mode (int fd) > +{ > + char buf[3] = "abc"; > + return fdopen (fd, buf); /* { dg-warning "stack-based buffer > over-read" } */ > + /* { dg-message "while looking for null terminator for argument 2 > \\('&buf'\\) of 'fdopen'..." "event" { target *-*-* } .-1 } */ > +} > + > +FILE * > +test_uninitialized_mode (int fd) > +{ > + char buf[10]; > + return fdopen (fd, buf); /* { dg-warning "use of uninitialized value > 'buf\\\[0\\\]'" } */ > + /* { dg-message "while looking for null terminator for argument 2 > \\('&buf'\\) of 'fdopen'..." "event" { target *-*-* } .-1 } */ > +} > + > +void > +test_leak (int fd) > +{ > + FILE *f = fdopen (fd, "r"); > +} /* { dg-warning "leak of FILE 'f'" } */ > + > +void > +test_double_fclose (int fd) > +{ > + FILE *f = fdopen (fd, "r"); > + if (!f) > + return; > + fclose (f); > + fclose (f); /* { dg-warning "double 'fclose' of FILE 'f'" } */ > +} Need test coverage for not complaining about a leak where we know we "own" the fd (due to an "open" call). > diff --git a/gcc/testsuite/gcc.dg/analyzer/fopen-1.c > b/gcc/testsuite/gcc.dg/analyzer/fopen-1.c > index f59d9efd3e1..2c7177374fa 100644 > --- a/gcc/testsuite/gcc.dg/analyzer/fopen-1.c > +++ b/gcc/testsuite/gcc.dg/analyzer/fopen-1.c > @@ -2,9 +2,17 @@ > string too long. > C++ compatible parts have been moved into > c-c++-common/analyzer/fopen-2.c */ > > +#include "analyzer-decls.h" > + > typedef struct FILE FILE; > FILE *fopen (const char *pathname, const char *mode); > > +FILE * > +test_null_mode (const char *pathname) > +{ > + return fopen (pathname, NULL); /* TODO: should warn about NULL mode. */ > +} > + Probably want a test_unterminated_mode here, if there isn't one already. > FILE * > test_unterminated_pathname (const char *mode) > { > diff --git a/gcc/testsuite/gcc.dg/analyzer/freopen-1.c > b/gcc/testsuite/gcc.dg/analyzer/freopen-1.c > new file mode 100644 > index 00000000000..d7b8d1a4331 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/analyzer/freopen-1.c > @@ -0,0 +1,114 @@ > +/* { dg-prune-output "-Wuse-after-free" } */ > + > +#include "analyzer-decls.h" > + > +typedef struct FILE FILE; > +FILE *freopen (const char *pathname, const char *mode, FILE *stream); > +int fclose (FILE *); > +int feof (FILE *); > + > +FILE * > +test_passthrough (const char *pathname, const char *mode, FILE *stream) > +{ > + return freopen (pathname, mode, stream); > +} > + > +FILE * > +test_null_pathname (const char *mode, FILE *stream) > +{ > + return freopen (NULL, mode, stream); > +} > + > +FILE * > +test_null_mode (const char *pathname, FILE *stream) > +{ > + return freopen (pathname, NULL, stream); /* TODO: should warn about > NULL mode. */ > +} > + > +FILE * > +test_simple_r (const char *pathname, FILE *stream) > +{ > + return freopen (pathname, "r", stream); > +} > + > +FILE * > +test_swapped_args (FILE *stream) > +{ > + return freopen ("r", "foo.txt", stream); /* TODO: would be nice to > detect this. */ > +} > + > +FILE * > +test_unterminated_mode (const char *pathname, FILE *stream) > +{ > + char buf[3] = "abc"; > + return freopen (pathname, buf, stream); /* { dg-warning "stack-based > buffer over-read" } */ > + /* { dg-message "while looking for null terminator for argument 2 > \\('&buf'\\) of 'freopen'..." "event" { target *-*-* } .-1 } */ > +} > + > +FILE * > +test_uninitialized_pathname (const char *mode, FILE *stream) > +{ > + char buf[10]; > + return freopen (buf, mode, stream); /* { dg-warning "use of > uninitialized value 'buf\\\[0\\\]'" } */ > + /* { dg-message "while looking for null terminator for argument 1 > \\('&buf'\\) of 'freopen'..." "event" { target *-*-* } .-1 } */ > +} > + > +FILE * > +test_uninitialized_mode (const char *pathname, FILE *stream) > +{ > + char buf[10]; > + return freopen (pathname, buf, stream); /* { dg-warning "use of > uninitialized value 'buf\\\[0\\\]'" } */ > + /* { dg-message "while looking for null terminator for argument 2 > \\('&buf'\\) of 'freopen'..." "event" { target *-*-* } .-1 } */ > +} > + > +void > +test_no_leak (const char *pathname, const char *mode, FILE *stream) > +{ > + /* Not a leak, f is the same as stream. */ > + FILE *f = freopen (pathname, mode, stream); > +} > + > +void > +test_double_fclose (const char *pathname, const char *mode, FILE *stream) > +{ > + FILE *f = freopen (pathname, mode, stream); > + if (!f) > + return; > + fclose (f); > + fclose (f); /* { dg-warning "double 'fclose' of FILE 'f'" } */ > +} > + > +void > +test_fclose_old_stream (const char *pathname, const char *mode, FILE > *stream) > +{ > + FILE *f = freopen (pathname, mode, stream); > + if (!f) > + return; > + /* f and stream alias the same FILE object, so this is the first > close. */ > + fclose (stream); > + fclose (f); /* { dg-warning "double 'fclose' of FILE 'f'" } */ > +} > + > +void > +test_freopen_after_fclose (const char *pathname, const char *mode, > + FILE *stream) > +{ > + fclose (stream); > + /* TODO: should warn about passing a closed stream to freopen. */ > + FILE *f = freopen (pathname, mode, stream); > + if (f) > + fclose (f); /* TODO: should warn, but the LHS transition overwrites > + stream's m_closed state, needs success/failure > + bifurcation. */ > +} > + > +/* f and stream alias the same FILE object, so using stream here is > valid. */ > +void > +test_use_old_stream (const char *pathname, const char *mode, FILE *stream) > +{ > + FILE *f = freopen (pathname, mode, stream); > + if (!f) > + return; > + feof (stream); > + fclose (f); > +} Thanks again Dave
