From patchwork Thu Jul 28 21:32:15 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 56405 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A6F19385BAF6 for ; Thu, 28 Jul 2022 21:32:48 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A6F19385BAF6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1659043968; bh=/viJHZd5yMebZ+guXVoPpco+/7PqiNMY7shLRrTQuPc=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=JDHNISRbA6s/w7MZBIawAT55jhhyeyM3GGZhz1De4omjpobiyfI7mwlvTXaIhoz/V 5yPeBiWcOjMsxL0TsUgZZOaA1rgNb+mGZgZb2gwF3zia5qcqeK0DGmFcy4Zq5+GHMy HtlAiXE9A2chFCxsafwwSi9nj5hOgMmzsNLqNKPY= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 8C4243856DFD for ; Thu, 28 Jul 2022 21:32:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8C4243856DFD Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-365-Qrp1j7U0PUygR-wvlDk89g-1; Thu, 28 Jul 2022 17:32:17 -0400 X-MC-Unique: Qrp1j7U0PUygR-wvlDk89g-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E290780A0B7 for ; Thu, 28 Jul 2022 21:32:16 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.2.16.236]) by smtp.corp.redhat.com (Postfix) with ESMTP id B7454492C3B; Thu, 28 Jul 2022 21:32:16 +0000 (UTC) To: gcc-patches@gcc.gnu.org Subject: [committed] analyzer: new warning: -Wanalyzer-putenv-of-auto-var [PR105893] Date: Thu, 28 Jul 2022 17:32:15 -0400 Message-Id: <20220728213215.2267605-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.85 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, URI_DOTEDU autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: David Malcolm via Gcc-patches From: David Malcolm Reply-To: David Malcolm Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" 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); | ^~~~~~~~~~~~ Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r13-1881-g872693eebb6b88. 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 5021376b6fb..808ff36ac54 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 8c38e9206fa..3f821ff07e1 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 f7df2fca245..a140f4d5088 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 42f8abeb043..a9657e0200a 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 5c6fdef16e3..186a76f5649 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 00000000000..4c3f0ae2e74 --- /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); +}