On Tue, 2026-03-10 at 15:33 +0100, Tomás Ortín Fernández (quanrong) wrote: > This patch adds two new analyzer warnings for misuse of mkstemp(3):
Hi Tomás, thanks for the patch. Overall, it looks great; I'm impressed. One legal prerequisite is that you need to either (a) add the Signed-off-by tag to certify that you can contribute it, as per https://developercertificate.org/ (relatively easy), or (b) jump through the legal hoops described here: https://gcc.gnu.org/contribute.html#legal (rather harder). Various other comments inline below: > > -Wanalyzer-mkstemp-of-string-literal warns when a string literal is > passed to mkstemp. Since mkstemp modifies its argument in place, > passing a string literal is undefined behavior (SEI CERT C rule > STR30-C). The diagnostic suggests using a writable character array > instead. > > -Wanalyzer-mkstemp-missing-suffix warns when the template argument > does not end with the required "XXXXXX" suffix. This addresses PR > analyzer/105890. > > Both warnings are enabled by default under -fanalyzer. > > The checks are in the analyzer rather than -Wformat because mkstemp > does not use a format attribute. Placing the checks in the analyzer > could also allow interprocedural analysis in the future, once the > analyzer can fully track string contents across function calls. > > Bootstrapped and tested on x86_64-pc-linux-gnu. Out of interest, how long did the bootstrap and testing take you? If that's a bottleneck, we may be able to set you up on a faster machine on the compile farm. > > gcc/analyzer/ChangeLog: > > PR analyzer/105890 > * analyzer.opt: Add -Wanalyzer-mkstemp-missing-suffix and > -Wanalyzer-mkstemp-of-string-literal. > * analyzer.opt.urls: Add URL entries for the new warnings. > * kf.cc (class mkstemp_of_string_literal): New diagnostic class > for mkstemp called on a string literal. > (class mkstemp_missing_suffix): New diagnostic class for mkstemp > called with a template missing the "XXXXXX" suffix. > (class kf_mkstemp): New known_function handler for mkstemp. > (register_known_functions): Register kf_mkstemp. > > gcc/testsuite/ChangeLog: > > * gcc.dg/analyzer/mkstemp-1.c: New test. > [...snip...] > diff --git a/gcc/analyzer/analyzer.opt.urls b/gcc/analyzer/analyzer.opt.urls > index 1a698f9c6d9..fe26d92f494 100644 > --- a/gcc/analyzer/analyzer.opt.urls > +++ b/gcc/analyzer/analyzer.opt.urls > @@ -84,6 +84,12 @@ > UrlSuffix(gcc/Static-Analyzer-Options.html#index-Wanalyzer-null-argument) > Wanalyzer-null-dereference > UrlSuffix(gcc/Static-Analyzer-Options.html#index-Wanalyzer-null-dereference) > > +Wanalyzer-mkstemp-missing-suffix > +UrlSuffix(gcc/Static-Analyzer-Options.html#index-Wanalyzer-mkstemp-missing-suffix) > + > +Wanalyzer-mkstemp-of-string-literal > +UrlSuffix(gcc/Static-Analyzer-Options.html#index-Wanalzyer-mkstemp-of-string-literal) > + We regenerate this file from the .opt files and the generated HTML via "make regenerate-opt-urls"; given that there aren't any docs for the new options and that there's a typo in the 2nd one, it looks like you hand-edited this instead. Please add docs for the new options to gcc/doc/invoke.texi (try searching for an existing analyzer warning to see what you need to add). Once that's done, "make regenerate-opt-urls" should rebuild the HTML and then regenerate analyzer.opt.urls for you. > Wanalyzer-putenv-of-auto-var > > UrlSuffix(gcc/Static-Analyzer-Options.html#index-Wanalyzer-putenv-of-auto-var) > > diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc > index b1ccbd6584a..173c52b58bd 100644 > --- a/gcc/analyzer/kf.cc > +++ b/gcc/analyzer/kf.cc > @@ -747,6 +747,190 @@ kf_memset::impl_call_pre (const call_details &cd) > const > cd.maybe_set_lhs (dest_sval); > } > [...snip...] > > + > +/* A subclass of pending_diagnostic for complaining about 'mkstemp' > + called with a template that does not end with XXXXXX. */ > + > +class mkstemp_missing_suffix > + : public pending_diagnostic_subclass<mkstemp_missing_suffix> > +{ > +public: > + mkstemp_missing_suffix (const call_details &cd) > + : m_call_stmt (cd.get_call_stmt ()), m_fndecl > (cd.get_fndecl_for_call ()) > + { > + gcc_assert (m_fndecl); > + } > + > + const char * > + get_kind () const final override > + { > + return "mkstemp_missing_suffix"; > + } > + > + bool > + operator== (const mkstemp_missing_suffix &other) const > + { > + return &m_call_stmt == &other.m_call_stmt; > + } > + > + int > + get_controlling_option () const final override > + { > + return OPT_Wanalyzer_mkstemp_missing_suffix; > + } > + > + bool > + emit (diagnostic_emission_context &ctxt) final override > + { > + return ctxt.warn ("%qE template does not end with XXXXXX", m_fndecl); Maybe say "template string" rather than just "template", to avoid confusion in C++ sources? (not sure about this one) A nitpick, XXXXXX is something that should be in the code, so we quote these. You could do: return ctxt.warn ("%qE template does not end with %qs", m_fndecl, "XXXXXX"); or return ctxt.warn ("%qE template does not end with %<XXXXXX>", m_fndecl); but the first is probably easier for translators. > + } > + > + bool > + describe_final_event (pretty_printer &pp, > + const evdesc::final_event &) final override > + { > + pp_printf (&pp, "%qE template does not end with XXXXXX", m_fndecl); > + return true; Likewise here. > + } > + > +private: > + const gimple &m_call_stmt; // non-NULL > + tree m_fndecl; // non-NULL > +}; > + > +/* Handler for calls to "mkstemp": > + > + int mkstemp(char *template); > + > + The template must not be a string constant, and its last six > + characters must be "XXXXXX". */ > + > +class kf_mkstemp : public known_function > +{ > +public: > + bool > + matches_call_types_p (const call_details &cd) const final override > + { > + return (cd.num_args () == 1 && cd.arg_is_pointer_p (0)); > + } > + > + void > + impl_call_pre (const call_details &cd) const final override > + { > + const svalue *ptr_sval = cd.get_arg_svalue (0); > + region_model_context *ctxt = cd.get_ctxt (); > + region_model *model = cd.get_model (); > + > + const svalue *strlen_sval > + = model->check_for_null_terminated_string_arg (cd, 0, false, > nullptr); > + if (!strlen_sval) > + { > + if (ctxt) > + ctxt->terminate_path (); > + return; > + } > + > + if (cd.get_arg_string_literal (0)) > + { > + if (ctxt) > + ctxt->warn (std::make_unique<mkstemp_of_string_literal> (cd)); > + } > + else if (check_suffix (cd, ptr_sval, strlen_sval).is_false ()) > + { > + if (ctxt) > + ctxt->warn (std::make_unique<mkstemp_missing_suffix> (cd)); > + } This looks correct, but it would be cleaner to do the if (ctxt) once, and have it guard a block that all the checks for problems live within. ctxt is non-null when the exploded graph is being built, but is null during feasibility checking. You can only store a warning if you have a non-null ctxt, so it's wasted work to check for a problem if there's no ctxt to report it to. > + > + cd.set_any_lhs_with_defaults (); > + } > + > +private: > + /* Return true if the template ends with "XXXXXX", false if it > + definitely does not, or unknown if we can't determine. */ > + static tristate > + check_suffix (const call_details &cd, const svalue *ptr_sval, > + const svalue *strlen_sval) > + { > + region_model *model = cd.get_model (); > + > + const constant_svalue *len_cst = > strlen_sval->dyn_cast_constant_svalue (); > + if (!len_cst) > + return tristate::TS_UNKNOWN; > + > + byte_offset_t len = TREE_INT_CST_LOW (len_cst->get_constant ()); > + if (len < 6) > + return tristate::TS_FALSE; > + > + tree arg_tree = cd.get_arg_tree (0); > + const region *reg > + = model->deref_rvalue (ptr_sval, arg_tree, cd.get_ctxt ()); > + const region *base_reg = reg->get_base_region (); > + const svalue *base_sval > + = model->get_store_value (base_reg, cd.get_ctxt ()); > + > + const constant_svalue *cst_sval = > base_sval->dyn_cast_constant_svalue (); > + if (!cst_sval) > + return tristate::TS_UNKNOWN; > + > + tree cst = cst_sval->get_constant (); > + if (TREE_CODE (cst) != STRING_CST) > + return tristate::TS_UNKNOWN; > + > + HOST_WIDE_INT str_len = len.to_shwi (); > + if (1 + str_len > TREE_STRING_LENGTH (cst)) > + return tristate::TS_UNKNOWN; > + > + if (memcmp (TREE_STRING_POINTER (cst) + str_len - 6, "XXXXXX", 6) != 0) > + return tristate::TS_FALSE; I'm not 100% convinced the logic here is correct, but I can't see a way to make it overflow the TREE_STRING_POINTER (cst) buffer. > + > + return tristate::TS_TRUE; > + } > +}; > + [...snip...] > diff --git a/gcc/testsuite/gcc.dg/analyzer/mkstemp-1.c > b/gcc/testsuite/gcc.dg/analyzer/mkstemp-1.c > new file mode 100644 > index 00000000000..2b6c82e3311 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/analyzer/mkstemp-1.c > @@ -0,0 +1,82 @@ > +/* { dg-additional-options "-Wno-analyzer-null-argument" } */ > +/* { dg-skip-if "has no putenv" { "avr-*-*" } } */ > + > +#include <stdio.h> > +#include <stdlib.h> > + > +extern void populate (char *buf); > + > +void test_passthrough (char *s) > +{ > + mkstemp (s); > +} > + > +void test_string_literal_correct_suffix (void) > +{ > + mkstemp ("/tmp/fooXXXXXX"); /* { dg-warning "'mkstemp' on a string > literal \\\[STR30-C\\\]" } */ > + /* { dg-message "use a writable character array" "fix suggestion" { > target *-*-* } .-1 } */ > +} > + > +void test_string_literal_missing_suffix (void) > +{ > + mkstemp ("/tmp/foo"); /* { dg-warning "'mkstemp' on a string literal > \\\[STR30-C\\\]" } */ > +} > + > +void test_string_literal_empty (void) > +{ > + mkstemp (""); /* { dg-warning "'mkstemp' on a string literal > \\\[STR30-C\\\]" } */ > +} > + > +void test_correct (void) > +{ > + char tmpl[] = "/tmp/fooXXXXXX"; > + mkstemp (tmpl); > +} > + > +void test_correct_minimal (void) > +{ > + char tmpl[] = "XXXXXX"; > + mkstemp (tmpl); > +} > + > +void test_missing_suffix (void) > +{ > + char tmpl[] = "/tmp/foo"; > + mkstemp (tmpl); /* { dg-warning "'mkstemp' template does not end with > XXXXXX" } */ > +} > + > +void test_too_short (void) > +{ > + char tmpl[] = "XXX"; > + mkstemp (tmpl); /* { dg-warning "'mkstemp' template does not end with > XXXXXX" } */ > +} > + > +void test_empty_buffer (void) > +{ > + char tmpl[] = ""; > + mkstemp (tmpl); /* { dg-warning "'mkstemp' template does not end with > XXXXXX" } */ > +} > + > +void test_partial_suffix (void) > +{ > + char tmpl[] = "/tmp/fooXXXXX_"; > + mkstemp (tmpl); /* { dg-warning "'mkstemp' template does not end with > XXXXXX" } */ > +} > + > +void test_five_xs (void) > +{ > + char tmpl[] = "/tmp/fooXXXXX"; > + mkstemp (tmpl); /* { dg-warning "'mkstemp' template does not end with > XXXXXX" } */ > +} > + > +void test_populated_buf (void) > +{ > + char tmpl[20]; > + populate (tmpl); > + mkstemp (tmpl); > +} > + > +void test_NULL (void) > +{ > + mkstemp (NULL); /* possibly -Wanalyzer-null-argument */ Indeed, check_for_null_terminated_string_arg would ideally check for this (I thought it did, but I see the TODO comment in the source; oh well). This patch is close to being ready, but, to repeat something I mentioned in an email to another candidate, right now we're in feature freeze for GCC 16, trying to focus on stabilizing for the release. We call this "stage 4" of the release cycle. Hence I plan to wait before actually pushing the patch. Hopefully we'll transition to stage 1 of the GCC 17 release cycle sometime next month (when we're open to new features). Thanks again for the patch; hope this is constructive Dave
