From patchwork Wed Jan 11 21:32:59 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 1724841 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=eEQdMWci; dkim-atps=neutral Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4NsgsQ1bBYz23fd for ; Thu, 12 Jan 2023 08:33:28 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 378863857C44 for ; Wed, 11 Jan 2023 21:33:26 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 378863857C44 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1673472806; bh=klJIuJDLDZRJ/IiRM2v1zBoo87EWR6/LI1/NZBSzVcA=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=eEQdMWciBidzcA9Tyj0AAGgrTXjqSNu3l1vQ64O9wJhgo67Be8iLUbsifNz4Rw7vH D/87oGGG2MSqf9GmH13LOVrcImGIgqpcrFyAxzhyA0gZfF7PKYztsoOrLTCpOWHp3h AHvS8HOMI4VO1NJ6UunXS9o/fyfQdzUgeZWQxavg= 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 DB19E3858C83 for ; Wed, 11 Jan 2023 21:33:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DB19E3858C83 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-247-U4rYm1rPM1CyVFlsC2EXBw-1; Wed, 11 Jan 2023 16:33:03 -0500 X-MC-Unique: U4rYm1rPM1CyVFlsC2EXBw-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id DF54029AA3B7 for ; Wed, 11 Jan 2023 21:33:02 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.2.16.103]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4B050492C14; Wed, 11 Jan 2023 21:33:02 +0000 (UTC) To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [committed] analyzer: fix leak false positives on "*UNKNOWN = PTR; " [PR108252] Date: Wed, 11 Jan 2023 16:32:59 -0500 Message-Id: <20230111213259.258216-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.9 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_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP 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+incoming=patchwork.ozlabs.org@gcc.gnu.org Sender: "Gcc-patches" PR analyzer/108252 reports a false positive from -Wanalyzer-malloc-leak on code like this: *ptr_ptr = strdup(EXPR); where ptr_ptr is an UNKNOWN_VALUE. When we handle: *UNKNOWN = PTR; store::set_value normally marks *PTR as having escaped, and this means we don't report PTR as leaking when the last usage of PTR is lost. However this only works for cases where PTR is a region_svalue. In the example in the bug, it's a conjured_svalue, rather than a region_svalue. A similar problem can arise for FDs, which aren't pointers. This patch fixes the bug by updating store::set_value to mark any values stored via *UNKNOWN = VAL as not leaking. Additionally, sm-malloc.cc's known_allocator_p hardcodes strdup and strndup as allocators (and thus transitioning their result to "unchecked"), but we don't implement known_functions for these, leading to the LHS being a CONJURED_SVALUE, rather than a region_svalue to a heap-allocated region. A similar issue happens with functions marked with __attribute__((malloc)). As part of a "belt and braces" fix, the patch also updates the handling of these functions, so that they use heap-allocated regions. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r13-5113-g688fc162b76dc6. gcc/analyzer/ChangeLog: PR analyzer/108252 * kf.cc (class kf_strdup): New. (class kf_strndup): New. (register_known_functions): Register them. * region-model.cc (region_model::on_call_pre): Use &HEAP_ALLOCATED_REGION for the default result of an external function with the "malloc" attribute, rather than CONJURED_SVALUE. (region_model::get_or_create_region_for_heap_alloc): Allow "size_in_bytes" to be NULL. * store.cc (store::set_value): When handling *UNKNOWN = VAL, mark VAL as "maybe bound". gcc/testsuite/ChangeLog: PR analyzer/108252 * gcc.dg/analyzer/attr-malloc-pr108252.c: New test. * gcc.dg/analyzer/fd-leak-pr108252.c: New test. * gcc.dg/analyzer/flex-with-call-summaries.c: Remove xfail from warning false +ve directives. * gcc.dg/analyzer/pr103217-2.c: Add -Wno-analyzer-too-complex. * gcc.dg/analyzer/pr103217-3.c: Likewise. * gcc.dg/analyzer/strdup-pr108252.c: New test. * gcc.dg/analyzer/strndup-pr108252.c: New test. Signed-off-by: David Malcolm --- gcc/analyzer/kf.cc | 56 +++++++++++++++++++ gcc/analyzer/region-model.cc | 32 +++++++---- gcc/analyzer/store.cc | 2 + .../gcc.dg/analyzer/attr-malloc-pr108252.c | 25 +++++++++ .../gcc.dg/analyzer/fd-leak-pr108252.c | 15 +++++ .../analyzer/flex-with-call-summaries.c | 6 +- gcc/testsuite/gcc.dg/analyzer/pr103217-2.c | 2 + gcc/testsuite/gcc.dg/analyzer/pr103217-3.c | 2 + .../gcc.dg/analyzer/strdup-pr108252.c | 19 +++++++ .../gcc.dg/analyzer/strndup-pr108252.c | 21 +++++++ 10 files changed, 166 insertions(+), 14 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/attr-malloc-pr108252.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-leak-pr108252.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/strdup-pr108252.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/strndup-pr108252.c diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc index 6088bfc72c0..53190c51772 100644 --- a/gcc/analyzer/kf.cc +++ b/gcc/analyzer/kf.cc @@ -851,6 +851,32 @@ kf_strcpy::impl_call_pre (const call_details &cd) const model->set_value (sized_dest_reg, src_contents_sval, cd.get_ctxt ()); } +/* Handler for "strdup" and "__builtin_strdup". */ + +class kf_strdup : 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 + { + region_model *model = cd.get_model (); + region_model_manager *mgr = cd.get_manager (); + /* Ideally we'd get the size here, and simulate copying the bytes. */ + const region *new_reg + = model->get_or_create_region_for_heap_alloc (NULL, cd.get_ctxt ()); + model->mark_region_as_unknown (new_reg, NULL); + if (cd.get_lhs_type ()) + { + const svalue *ptr_sval + = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg); + cd.maybe_set_lhs (ptr_sval); + } + } +}; + /* Handle the on_call_pre part of "strlen". */ class kf_strlen : public known_function @@ -892,6 +918,32 @@ kf_strlen::impl_call_pre (const call_details &cd) const /* Otherwise a conjured value. */ } +/* Handler for "strndup" and "__builtin_strndup". */ + +class kf_strndup : public known_function +{ +public: + bool matches_call_types_p (const call_details &cd) const final override + { + return (cd.num_args () == 2 && cd.arg_is_pointer_p (0)); + } + void impl_call_pre (const call_details &cd) const final override + { + region_model *model = cd.get_model (); + region_model_manager *mgr = cd.get_manager (); + /* Ideally we'd get the size here, and simulate copying the bytes. */ + const region *new_reg + = model->get_or_create_region_for_heap_alloc (NULL, cd.get_ctxt ()); + model->mark_region_as_unknown (new_reg, NULL); + if (cd.get_lhs_type ()) + { + const svalue *ptr_sval + = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg); + cd.maybe_set_lhs (ptr_sval); + } + } +}; + class kf_ubsan_bounds : public internal_known_function { /* Empty. */ @@ -943,6 +995,8 @@ register_known_functions (known_function_manager &kfm) kfm.add (BUILT_IN_STRCHR, make_unique ()); kfm.add (BUILT_IN_STRCPY, make_unique (2)); kfm.add (BUILT_IN_STRCPY_CHK, make_unique (3)); + kfm.add (BUILT_IN_STRDUP, make_unique ()); + kfm.add (BUILT_IN_STRNDUP, make_unique ()); kfm.add (BUILT_IN_STRLEN, make_unique ()); register_varargs_builtins (kfm); @@ -951,6 +1005,8 @@ register_known_functions (known_function_manager &kfm) /* Known builtins and C standard library functions. */ { kfm.add ("memset", make_unique ()); + kfm.add ("strdup", make_unique ()); + kfm.add ("strndup", make_unique ()); } /* Known POSIX functions, and some non-standard extensions. */ diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 55064400a08..2e59fbaadd7 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -1441,6 +1441,8 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt) if (ctxt) check_call_args (cd); + tree callee_fndecl = get_fndecl_for_call (call, ctxt); + /* Some of the cases below update the lhs of the call based on the return value, but not all. Provide a default value, which may get overwritten below. */ @@ -1450,13 +1452,22 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt) const svalue *sval = maybe_get_const_fn_result (cd); if (!sval) { - /* For the common case of functions without __attribute__((const)), - use a conjured value, and purge any prior state involving that - value (in case this is in a loop). */ - sval = m_mgr->get_or_create_conjured_svalue (TREE_TYPE (lhs), call, - lhs_region, - conjured_purge (this, - ctxt)); + if (callee_fndecl + && lookup_attribute ("malloc", DECL_ATTRIBUTES (callee_fndecl))) + { + const region *new_reg + = get_or_create_region_for_heap_alloc (NULL, ctxt); + mark_region_as_unknown (new_reg, NULL); + sval = m_mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg); + } + else + /* For the common case of functions without __attribute__((const)), + use a conjured value, and purge any prior state involving that + value (in case this is in a loop). */ + sval = m_mgr->get_or_create_conjured_svalue (TREE_TYPE (lhs), call, + lhs_region, + conjured_purge (this, + ctxt)); } set_value (lhs_region, sval, ctxt); } @@ -1469,7 +1480,7 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt) return false; } - if (tree callee_fndecl = get_fndecl_for_call (call, ctxt)) + if (callee_fndecl) { int callee_fndecl_flags = flags_from_decl_or_type (callee_fndecl); @@ -4909,8 +4920,9 @@ region_model::get_or_create_region_for_heap_alloc (const svalue *size_in_bytes, get_referenced_base_regions (base_regs_in_use); const region *reg = m_mgr->get_or_create_region_for_heap_alloc (base_regs_in_use); - if (compat_types_p (size_in_bytes->get_type (), size_type_node)) - set_dynamic_extents (reg, size_in_bytes, ctxt); + if (size_in_bytes) + if (compat_types_p (size_in_bytes->get_type (), size_type_node)) + set_dynamic_extents (reg, size_in_bytes, ctxt); return reg; } diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc index f3b500c50a0..6ad7110a53c 100644 --- a/gcc/analyzer/store.cc +++ b/gcc/analyzer/store.cc @@ -2578,6 +2578,8 @@ store::set_value (store_manager *mgr, const region *lhs_reg, const region *ptr_base_reg = ptr_dst->get_base_region (); mark_as_escaped (ptr_base_reg); } + if (uncertainty) + uncertainty->on_maybe_bound_sval (rhs_sval); } else if (lhs_base_reg->tracked_p ()) { diff --git a/gcc/testsuite/gcc.dg/analyzer/attr-malloc-pr108252.c b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-pr108252.c new file mode 100644 index 00000000000..5e3437985cb --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-pr108252.c @@ -0,0 +1,25 @@ +struct foo +{ + int m_int; +}; + +extern void foo_release (struct foo *); +extern struct foo *foo_acquire (void) + __attribute__ ((malloc (foo_release))); + +struct { + /* [...snip...] */ + struct foo *listen_default_ciphers; + struct foo *connect_default_ciphers; + /* [...snip...] */ +} g; + +int parse_global_ciphers(char **args) +{ + struct foo **target; + target = ((args[0][12] == 'b') + ? &g.listen_default_ciphers + : &g.connect_default_ciphers); + *target = foo_acquire (); + return 0; /* { dg-bogus "leak" } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-leak-pr108252.c b/gcc/testsuite/gcc.dg/analyzer/fd-leak-pr108252.c new file mode 100644 index 00000000000..29f58d361c1 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/fd-leak-pr108252.c @@ -0,0 +1,15 @@ +extern int open(const char *, int mode); +#define O_RDONLY 0 + +struct { + int fd_a; + int fd_b; +} g; + +int test (const char *path, int flag) +{ + int *target; + target = flag ? &g.fd_a : &g.fd_b; + *target = open (path, O_RDONLY); + return 0; /* { dg-bogus "leak of file descriptor" } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/flex-with-call-summaries.c b/gcc/testsuite/gcc.dg/analyzer/flex-with-call-summaries.c index 00566d58418..0ff652b427b 100644 --- a/gcc/testsuite/gcc.dg/analyzer/flex-with-call-summaries.c +++ b/gcc/testsuite/gcc.dg/analyzer/flex-with-call-summaries.c @@ -913,8 +913,7 @@ static int yy_get_next_buffer (void) if ( number_to_move == YY_MORE_ADJ ) { ret_val = EOB_ACT_END_OF_FILE; - yyrestart( yyin );/* { dg-bogus "leak" "" { xfail *-*-* } } */ - /* TODO: leak false positive: PR analyzer/103546. */ + yyrestart( yyin );/* { dg-bogus "leak" } */ } else @@ -1101,8 +1100,7 @@ static int yy_get_next_buffer (void) #ifdef __cplusplus return yyinput(); #else - return input(); /* { dg-bogus "infinite recursion" "" { xfail *-*-* } } */ - /* TODO: infinite recursion positive: PR analyzer/103546. */ + return input(); /* { dg-bogus "infinite recursion" } */ #endif } diff --git a/gcc/testsuite/gcc.dg/analyzer/pr103217-2.c b/gcc/testsuite/gcc.dg/analyzer/pr103217-2.c index 3a9c4145848..aa8bca7ce5f 100644 --- a/gcc/testsuite/gcc.dg/analyzer/pr103217-2.c +++ b/gcc/testsuite/gcc.dg/analyzer/pr103217-2.c @@ -1,3 +1,5 @@ +/* { dg-additional-options "-Wno-analyzer-too-complex" } */ + typedef __SIZE_TYPE__ size_t; extern void *calloc (size_t __nmemb, size_t __size) diff --git a/gcc/testsuite/gcc.dg/analyzer/pr103217-3.c b/gcc/testsuite/gcc.dg/analyzer/pr103217-3.c index b103a121650..e5f1e4bd62d 100644 --- a/gcc/testsuite/gcc.dg/analyzer/pr103217-3.c +++ b/gcc/testsuite/gcc.dg/analyzer/pr103217-3.c @@ -1,3 +1,5 @@ +/* { dg-additional-options "-Wno-analyzer-too-complex" } */ + extern char *strdup (const char *__s) __attribute__ ((__nothrow__ , __leaf__, __malloc__, __nonnull__ (1))); diff --git a/gcc/testsuite/gcc.dg/analyzer/strdup-pr108252.c b/gcc/testsuite/gcc.dg/analyzer/strdup-pr108252.c new file mode 100644 index 00000000000..d79d11fca66 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/strdup-pr108252.c @@ -0,0 +1,19 @@ +extern char *strdup (const char *__s) + __attribute__ ((__nothrow__ , __leaf__, __malloc__, __nonnull__ (1))); + +struct { + /* [...snip...] */ + char *listen_default_ciphers; + char *connect_default_ciphers; + /* [...snip...] */ +} g; + +int parse_global_ciphers(char **args) +{ + char **target; + target = ((args[0][12] == 'b') + ? &g.listen_default_ciphers + : &g.connect_default_ciphers); + *target = strdup(args[1]); + return 0; /* { dg-bogus "leak" } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/strndup-pr108252.c b/gcc/testsuite/gcc.dg/analyzer/strndup-pr108252.c new file mode 100644 index 00000000000..3e64f27c29a --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/strndup-pr108252.c @@ -0,0 +1,21 @@ +typedef __SIZE_TYPE__ size_t; + +extern char *strndup (const char *__s, size_t sz) + __attribute__ ((__nothrow__ , __leaf__, __malloc__, __nonnull__ (1))); + +struct { + /* [...snip...] */ + char *listen_default_ciphers; + char *connect_default_ciphers; + /* [...snip...] */ +} g; + +int parse_global_ciphers(char **args) +{ + char **target; + target = ((args[0][12] == 'b') + ? &g.listen_default_ciphers + : &g.connect_default_ciphers); + *target = strndup(args[1], 42); + return 0; /* { dg-bogus "leak" } */ +}