From 872693eebb6b88f4b6a2767727a9565d05172768 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Thu, 28 Jul 2022 17:21:29 -0400 Subject: [PATCH] analyzer: new warning: -Wanalyzer-putenv-of-auto-var [PR105893] MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This patch implements a new -fanalyzer warning: -Wanalyzer-putenv-of-auto-var which complains about stack pointers passed to putenv(3) calls, as per SEI CERT C Coding Standard rule POS34-C ("Do not call putenv() with a pointer to an automatic variable as the argument"). For example, given: #include #include void test_arr (void) { char arr[] = "NAME=VALUE"; putenv (arr); } it emits: demo.c: In function ‘test_arr’: demo.c:7:3: warning: ‘putenv’ on a pointer to automatic variable ‘arr’ [POS34-C] [-Wanalyzer-putenv-of-auto-var] 7 | putenv (arr); | ^~~~~~~~~~~~ ‘test_arr’: event 1 | | 7 | putenv (arr); | | ^~~~~~~~~~~~ | | | | | (1) ‘putenv’ on a pointer to automatic variable ‘arr’ | demo.c:6:8: note: ‘arr’ declared on stack here 6 | char arr[] = "NAME=VALUE"; | ^~~ demo.c:7:3: note: perhaps use ‘setenv’ rather than ‘putenv’ 7 | putenv (arr); | ^~~~~~~~~~~~ gcc/analyzer/ChangeLog: PR analyzer/105893 * analyzer.opt (Wanalyzer-putenv-of-auto-var): New. * region-model-impl-calls.cc (class putenv_of_auto_var): New. (region_model::impl_call_putenv): New. * region-model.cc (region_model::on_call_pre): Handle putenv. * region-model.h (region_model::impl_call_putenv): New decl. gcc/ChangeLog: PR analyzer/105893 * doc/invoke.texi: Add -Wanalyzer-putenv-of-auto-var. gcc/testsuite/ChangeLog: PR analyzer/105893 * gcc.dg/analyzer/putenv-1.c: New test. Signed-off-by: David Malcolm --- gcc/analyzer/analyzer.opt | 4 ++ gcc/analyzer/region-model-impl-calls.cc | 117 +++++++++++++++++++++++++++++++ gcc/analyzer/region-model.cc | 6 ++ gcc/analyzer/region-model.h | 1 + gcc/doc/invoke.texi | 14 ++++ gcc/testsuite/gcc.dg/analyzer/putenv-1.c | 109 ++++++++++++++++++++++++++++ 6 files changed, 251 insertions(+) create mode 100644 gcc/testsuite/gcc.dg/analyzer/putenv-1.c diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt index 5021376..808ff36 100644 --- a/gcc/analyzer/analyzer.opt +++ b/gcc/analyzer/analyzer.opt @@ -126,6 +126,10 @@ Wanalyzer-null-dereference Common Var(warn_analyzer_null_dereference) Init(1) Warning Warn about code paths in which a NULL pointer is dereferenced. +Wanalyzer-putenv-of-auto-var +Common Var(warn_analyzer_putenv_of_auto_var) Init(1) Warning +Warn about code paths in which an on-stack buffer is passed to putenv. + Wanalyzer-shift-count-negative Common Var(warn_analyzer_shift_count_negative) Init(1) Warning Warn about code paths in which a shift with negative count is attempted. diff --git a/gcc/analyzer/region-model-impl-calls.cc b/gcc/analyzer/region-model-impl-calls.cc index 8c38e92..3f821ff 100644 --- a/gcc/analyzer/region-model-impl-calls.cc +++ b/gcc/analyzer/region-model-impl-calls.cc @@ -549,6 +549,123 @@ region_model::impl_call_memset (const call_details &cd) fill_region (sized_dest_reg, fill_value_u8); } +/* A subclass of pending_diagnostic for complaining about 'putenv' + called on an auto var. */ + +class putenv_of_auto_var +: public pending_diagnostic_subclass +{ +public: + putenv_of_auto_var (tree fndecl, const region *reg) + : m_fndecl (fndecl), m_reg (reg), + m_var_decl (reg->get_base_region ()->maybe_get_decl ()) + { + } + + const char *get_kind () const final override + { + return "putenv_of_auto_var"; + } + + bool operator== (const putenv_of_auto_var &other) const + { + return (m_fndecl == other.m_fndecl + && m_reg == other.m_reg + && same_tree_p (m_var_decl, other.m_var_decl)); + } + + int get_controlling_option () const final override + { + return OPT_Wanalyzer_putenv_of_auto_var; + } + + bool emit (rich_location *rich_loc) final override + { + auto_diagnostic_group d; + diagnostic_metadata m; + + /* SEI CERT C Coding Standard: "POS34-C. Do not call putenv() with a + pointer to an automatic variable as the argument". */ + diagnostic_metadata::precanned_rule + rule ("POS34-C", "https://wiki.sei.cmu.edu/confluence/x/6NYxBQ"); + m.add_rule (rule); + + bool warned; + if (m_var_decl) + warned = warning_meta (rich_loc, m, get_controlling_option (), + "%qE on a pointer to automatic variable %qE", + m_fndecl, m_var_decl); + else + warned = warning_meta (rich_loc, m, get_controlling_option (), + "%qE on a pointer to an on-stack buffer", + m_fndecl); + if (warned) + { + if (m_var_decl) + inform (DECL_SOURCE_LOCATION (m_var_decl), + "%qE declared on stack here", m_var_decl); + inform (rich_loc->get_loc (), "perhaps use %qs rather than %qE", + "setenv", m_fndecl); + } + + return warned; + } + + label_text describe_final_event (const evdesc::final_event &ev) final override + { + if (m_var_decl) + return ev.formatted_print ("%qE on a pointer to automatic variable %qE", + m_fndecl, m_var_decl); + else + return ev.formatted_print ("%qE on a pointer to an on-stack buffer", + m_fndecl); + } + + void mark_interesting_stuff (interesting_t *interest) final override + { + if (!m_var_decl) + interest->add_region_creation (m_reg->get_base_region ()); + } + +private: + tree m_fndecl; // non-NULL + const region *m_reg; // non-NULL + tree m_var_decl; // could be NULL +}; + +/* Handle the on_call_pre part of "putenv". + + In theory we could try to model the state of the environment variables + for the process; for now we merely complain about putenv of regions + on the stack. */ + +void +region_model::impl_call_putenv (const call_details &cd) +{ + tree fndecl = cd.get_fndecl_for_call (); + gcc_assert (fndecl); + region_model_context *ctxt = cd.get_ctxt (); + const svalue *ptr_sval = cd.get_arg_svalue (0); + const region *reg = deref_rvalue (ptr_sval, cd.get_arg_tree (0), ctxt); + m_store.mark_as_escaped (reg); + enum memory_space mem_space = reg->get_memory_space (); + switch (mem_space) + { + default: + gcc_unreachable (); + case MEMSPACE_UNKNOWN: + case MEMSPACE_CODE: + case MEMSPACE_GLOBALS: + case MEMSPACE_HEAP: + case MEMSPACE_READONLY_DATA: + break; + case MEMSPACE_STACK: + if (ctxt) + ctxt->warn (new putenv_of_auto_var (fndecl, reg)); + break; + } +} + /* Handle the on_call_pre part of "operator new". */ void diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index f7df2fc..a140f4d 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -1539,6 +1539,12 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt, impl_call_memset (cd); return false; } + else if (is_named_call_p (callee_fndecl, "putenv", call, 1) + && POINTER_TYPE_P (cd.get_arg_type (0))) + { + impl_call_putenv (cd); + return false; + } else if (is_named_call_p (callee_fndecl, "strchr", call, 2) && POINTER_TYPE_P (cd.get_arg_type (0))) { diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index 42f8abe..a9657e0 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -630,6 +630,7 @@ class region_model void impl_call_malloc (const call_details &cd); void impl_call_memcpy (const call_details &cd); void impl_call_memset (const call_details &cd); + void impl_call_putenv (const call_details &cd); void impl_call_realloc (const call_details &cd); void impl_call_strchr (const call_details &cd); void impl_call_strcpy (const call_details &cd); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 38ae900..e8cd601 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -459,6 +459,7 @@ Objective-C and Objective-C++ Dialects}. -Wno-analyzer-null-dereference @gol -Wno-analyzer-possible-null-argument @gol -Wno-analyzer-possible-null-dereference @gol +-Wno-analyzer-putenv-of-auto-var @gol -Wno-analyzer-shift-count-negative @gol -Wno-analyzer-shift-count-overflow @gol -Wno-analyzer-stale-setjmp-buffer @gol @@ -9761,6 +9762,7 @@ Enabling this option effectively enables the following warnings: -Wanalyzer-null-dereference @gol -Wanalyzer-possible-null-argument @gol -Wanalyzer-possible-null-dereference @gol +-Wanalyzer-putenv-of-auto-var @gol -Wanalyzer-shift-count-negative @gol -Wanalyzer-shift-count-overflow @gol -Wanalyzer-stale-setjmp-buffer @gol @@ -10017,6 +10019,18 @@ value known to be NULL is dereferenced. See @uref{https://cwe.mitre.org/data/definitions/476.html, CWE-476: NULL Pointer Dereference}. +@item -Wno-analyzer-putenv-of-auto-var +@opindex Wanalyzer-putenv-of-auto-var +@opindex Wno-analyzer-putenv-of-auto-var +This warning requires @option{-fanalyzer}, which enables it; use +@option{-Wno-analyzer-possible-null-dereference} to disable it. + +This diagnostic warns for paths through the code in which a +call to @code{putenv} is passed a pointer to an automatic variable +or an on-stack buffer. + +See @uref{https://wiki.sei.cmu.edu/confluence/x/6NYxBQ, POS34-C. Do not call putenv() with a pointer to an automatic variable as the argument}. + @item -Wno-analyzer-shift-count-negative @opindex Wanalyzer-shift-count-negative @opindex Wno-analyzer-shift-count-negative diff --git a/gcc/testsuite/gcc.dg/analyzer/putenv-1.c b/gcc/testsuite/gcc.dg/analyzer/putenv-1.c new file mode 100644 index 0000000..4c3f0ae --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/putenv-1.c @@ -0,0 +1,109 @@ +/* { dg-additional-options "-Wno-analyzer-null-argument" } */ + +#include +#include + +extern void populate (char *buf); + +void test_passthrough (char *s) +{ + putenv (s); +} + +void test_str_lit (void) +{ + putenv ("NAME=value"); +} + +/* glibc allows strings without an equal sign. */ + +void test_no_eq (void) +{ + putenv ("NAME"); +} + +void test_empty_string (void) +{ + putenv (""); +} + +void test_NULL (void) +{ + putenv (NULL); /* possibly -Wanalyzer-null-argument */ +} + +void test_auto_buf_name_and_value (const char *name, const char *value) +{ + char buf[100]; /* { dg-message "'buf' declared on stack here" } */ + snprintf (buf, sizeof (buf), "%s=%s", name, value); + putenv (buf); /* { dg-warning "'putenv' on a pointer to automatic variable 'buf' \\\[POS34-C\\\]" "warning" } */ + /* { dg-message "perhaps use 'setenv' rather than 'putenv'" "setenv suggestion" { target *-*-* } .-1 } */ +} + +void test_auto_buf_value (const char *value) +{ + char buf[100]; /* { dg-message "'buf' declared on stack here" } */ + snprintf (buf, sizeof (buf), "NAME=%s", value); + putenv (buf); /* { dg-warning "'putenv' on a pointer to automatic variable 'buf' \\\[POS34-C\\\]" } */ +} + +void test_static_buf (const char *value) +{ + static char buf[100]; + snprintf (buf, sizeof (buf), "NAME=%s", value); + putenv (buf); +} + +static char global_buf[1024]; + +void test_global (const char *value) +{ + snprintf (global_buf, sizeof (global_buf), "NAME=%s", value); + putenv (global_buf); +} + +void test_alloca (void) +{ + char *buf = __builtin_alloca (256); /* { dg-message "region created on stack here" } */ + populate (buf); + putenv (buf); /* { dg-warning "'putenv' on a pointer to an on-stack buffer \\\[POS34-C\\\]" } */ +} + +void test_malloc_1 (void) +{ + char *buf = malloc (1024); + if (!buf) + return; + populate (buf); + putenv (buf); +} + +void test_malloc_2 (void) +{ + const char *kvstr = "NAME=value"; + size_t len = __builtin_strlen (kvstr); + char *buf = __builtin_malloc (len + 1); + if (!buf) + return; + __builtin_memcpy (buf, kvstr, len); + buf[len] = '\0'; + putenv (buf); /* { dg-bogus "leak" } */ +} + +void test_arr (void) +{ + char arr[] = "NAME=VALUE"; /* { dg-message "'arr' declared on stack here" } */ + putenv (arr); /* { dg-warning "'putenv' on a pointer to automatic variable 'arr' \\\[POS34-C\\\]" } */ +} + +static void __attribute__((noinline)) +__analyzer_test_inner (char *kvstr) +{ + putenv (kvstr); /* { dg-warning "'putenv' on a pointer to automatic variable 'arr_outer' \\\[POS34-C\\\]" } */ +} + +void test_outer (void) +{ + char arr_outer[] = "NAME=VALUE"; /* { dg-message "'arr_outer' declared on stack here" } */ + __analyzer_test_inner (arr_outer); +} -- 2.7.4