From patchwork Mon Nov 29 23:53:30 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 48268 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 E645E3858023 for ; Mon, 29 Nov 2021 23:54:08 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E645E3858023 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1638230048; bh=95m3K8UTgIChpsvaG8TXLpt0RZB4/DXDtZMSRYnA4a0=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=ipewkIXomn+WIEcFy66mgUL/L7boHUhUnK6GPd2qBbKUdxx5upMSuZ5Xfkb4iHv5Z KcXYrnHpILEOnBGmAFVxu3Efk0WnWL44yl0nLRpYQbmh/TwBsU8ynRHMJsEEZAzfMR ItahSxMgvN0OXfVM0oSLsL1J2W5X0eboo6NYHzY8= 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.133.124]) by sourceware.org (Postfix) with ESMTPS id EF7683858402 for ; Mon, 29 Nov 2021 23:53:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EF7683858402 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-424-3AyHm1oAPR6wGpYx1S4HuQ-1; Mon, 29 Nov 2021 18:53:34 -0500 X-MC-Unique: 3AyHm1oAPR6wGpYx1S4HuQ-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C0E3C1030C25 for ; Mon, 29 Nov 2021 23:53:33 +0000 (UTC) Received: from t14s.localdomain.com (ovpn-112-37.phx2.redhat.com [10.3.112.37]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3FD9360854; Mon, 29 Nov 2021 23:53:33 +0000 (UTC) To: gcc-patches@gcc.gnu.org Subject: [committed] analyzer: further false leak fixes due to overzealous state merging [PR103217] Date: Mon, 29 Nov 2021 18:53:30 -0500 Message-Id: <20211129235330.3127723-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-13.3 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, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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" Commit r12-5424-gf573d35147ca8433c102e1721d8c99fc432cb44b fixed a false positive from -Wanalyzer-malloc-leak due to overzealous state merging, erroneously merging two different svalues bound to a particular part of the store when one has sm-state. A further case was discovered by the reporter of PR analyzer/103217, which this patch fixes. In this variant, different states have set different fields of a struct, and on attempting to merge them, the states have a different set of binding keys, leading to one state having an svalue with sm-state, and its peer state having a NULL value for that binding key. The state merger code was erroneously treating them as mergeable to "UNKNOWN". This followup patch fixes things by rejecting such mergers if the non-NULL svalue is not mergeable with "UNKNOWN". Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r12-5585-g132902177138c09803d639e12b1daebf2b9edddc. gcc/analyzer/ChangeLog: PR analyzer/103217 * store.cc (binding_cluster::can_merge_p): For the "key is bound" vs "key is not bound" merger case, check that the bound svalue is mergeable before merging it to "unknown", rejecting the merger otherwise. gcc/testsuite/ChangeLog: PR analyzer/103217 * gcc.dg/analyzer/pr103217-2.c: New test. * gcc.dg/analyzer/pr103217-3.c: New test. * gcc.dg/analyzer/pr103217-4.c: New test. * gcc.dg/analyzer/pr103217-5.c: New test. Signed-off-by: David Malcolm --- gcc/analyzer/store.cc | 14 +++++- gcc/testsuite/gcc.dg/analyzer/pr103217-2.c | 52 ++++++++++++++++++++++ gcc/testsuite/gcc.dg/analyzer/pr103217-3.c | 52 ++++++++++++++++++++++ gcc/testsuite/gcc.dg/analyzer/pr103217-4.c | 52 ++++++++++++++++++++++ gcc/testsuite/gcc.dg/analyzer/pr103217-5.c | 47 +++++++++++++++++++ 5 files changed, 215 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr103217-2.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr103217-3.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr103217-4.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr103217-5.c diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc index 3760858c26d..5eecbe6cf18 100644 --- a/gcc/analyzer/store.cc +++ b/gcc/analyzer/store.cc @@ -1729,6 +1729,7 @@ binding_cluster::can_merge_p (const binding_cluster *cluster_a, for (hash_set::iterator iter = keys.begin (); iter != keys.end (); ++iter) { + region_model_manager *sval_mgr = mgr->get_svalue_manager (); const binding_key *key = *iter; const svalue *sval_a = cluster_a->get_any_value (key); const svalue *sval_b = cluster_b->get_any_value (key); @@ -1746,7 +1747,6 @@ binding_cluster::can_merge_p (const binding_cluster *cluster_a, } else if (sval_a && sval_b) { - region_model_manager *sval_mgr = mgr->get_svalue_manager (); if (const svalue *merged_sval = sval_a->can_merge_p (sval_b, sval_mgr, merger)) { @@ -1760,9 +1760,19 @@ binding_cluster::can_merge_p (const binding_cluster *cluster_a, /* If we get here, then one cluster binds this key and the other doesn't; merge them as "UNKNOWN". */ gcc_assert (sval_a || sval_b); - tree type = sval_a ? sval_a->get_type () : sval_b->get_type (); + + const svalue *bound_sval = sval_a ? sval_a : sval_b; + tree type = bound_sval->get_type (); const svalue *unknown_sval = mgr->get_svalue_manager ()->get_or_create_unknown_svalue (type); + + /* ...but reject the merger if this sval shouldn't be mergeable + (e.g. reject merging svalues that have non-purgable sm-state, + to avoid falsely reporting memory leaks by merging them + with something else). */ + if (!bound_sval->can_merge_p (unknown_sval, sval_mgr, merger)) + return false; + out_cluster->m_map.put (key, unknown_sval); } diff --git a/gcc/testsuite/gcc.dg/analyzer/pr103217-2.c b/gcc/testsuite/gcc.dg/analyzer/pr103217-2.c new file mode 100644 index 00000000000..3a9c4145848 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr103217-2.c @@ -0,0 +1,52 @@ +typedef __SIZE_TYPE__ size_t; + +extern void *calloc (size_t __nmemb, size_t __size) + __attribute__ ((__nothrow__ , __leaf__, __malloc__, __alloc_size__ (1, 2))); + +extern char *strdup (const char *__s) + __attribute__ ((__nothrow__ , __leaf__, __malloc__, __nonnull__ (1))); + +extern void abort (void) + __attribute__ ((__nothrow__ , __leaf__, __noreturn__)); + +extern int getopt (int ___argc, char *const *___argv, const char *__shortopts) + __attribute__ ((__nothrow__ , __leaf__, __nonnull__ (2, 3))); +extern char *optarg; + +extern void free (void *__ptr) + __attribute__ ((__nothrow__ , __leaf__)); + +char *xstrdup(const char *src) { + char *val = strdup(src); + if (!val) + abort(); + return val; +} + +struct test { + char *one, *two; +}; + +int main(int argc, char *argv[]) { + struct test *options = calloc(1, sizeof(*options)); + int rc; + if (!options) + abort(); + + while ((rc = getopt(argc, argv, "a:b:")) != -1) { + switch (rc) { + case 'a': + free(options->one); + options->one = xstrdup(optarg); + break; + case 'b': + free(options->two); + options->two = xstrdup(optarg); + break; + } + } + free(options->one); + free(options->two); + free(options); + return 0; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr103217-3.c b/gcc/testsuite/gcc.dg/analyzer/pr103217-3.c new file mode 100644 index 00000000000..b103a121650 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr103217-3.c @@ -0,0 +1,52 @@ +extern char *strdup (const char *__s) + __attribute__ ((__nothrow__ , __leaf__, __malloc__, __nonnull__ (1))); + +extern void abort (void) + __attribute__ ((__nothrow__ , __leaf__, __noreturn__)); + +extern int getopt (int ___argc, char *const *___argv, const char *__shortopts) + __attribute__ ((__nothrow__ , __leaf__, __nonnull__ (2, 3))); +extern char *optarg; + +extern void free (void *__ptr) + __attribute__ ((__nothrow__ , __leaf__)); + +struct state { + const char *confpath; + const char *host; + const char *port; + const char *state_dir_prefix; +}; + +static inline char *xstrdup(const char *s) { + char *val = strdup(s); + if (!val) + abort(); + return val; +} + +int config_init(struct state *config); + +int main(int argc, char *argv[]) { + int rc; + struct state state = { 0 }; + + config_init(&state); + + while ((rc = getopt(argc, argv, "H:p:")) != -1) { + switch (rc) { + case 'H': + free((void*)state.host); + state.host = xstrdup(optarg); + break; + case 'p': + free((void*)state.port); + state.port = xstrdup(optarg); + break; + } + } + + free((void*)state.host); + free((void*)state.port); + return rc; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr103217-4.c b/gcc/testsuite/gcc.dg/analyzer/pr103217-4.c new file mode 100644 index 00000000000..516e1f4f997 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr103217-4.c @@ -0,0 +1,52 @@ +extern char *strdup (const char *__s) + __attribute__ ((__nothrow__ , __leaf__, __malloc__, __nonnull__ (1))); + +extern void abort (void) + __attribute__ ((__nothrow__ , __leaf__, __noreturn__)); + +extern int getopt (int ___argc, char *const *___argv, const char *__shortopts) + __attribute__ ((__nothrow__ , __leaf__, __nonnull__ (2, 3))); +extern char *optarg; + +extern void free (void *__ptr) + __attribute__ ((__nothrow__ , __leaf__)); + +struct state { + const char *confpath; + const char *host; + const char *port; + const char *state_dir_prefix; +}; + +static inline char *xstrdup(const char *s) { + char *val = strdup(s); + if (!val) + abort(); + return val; +} + +int config_init(struct state *config); + +int main(int argc, char *argv[]) { + int rc; + struct state state = { 0 }; + + config_init(&state); + + if ((rc = getopt(argc, argv, "H:p:")) != -1) { + switch (rc) { + case 'H': + free((void*)state.host); + state.host = xstrdup(optarg); + break; + case 'p': + free((void*)state.port); + state.port = xstrdup(optarg); + break; + } + } + + free((void*)state.host); + free((void*)state.port); + return rc; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr103217-5.c b/gcc/testsuite/gcc.dg/analyzer/pr103217-5.c new file mode 100644 index 00000000000..d916d92eeec --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr103217-5.c @@ -0,0 +1,47 @@ +extern char *strdup (const char *__s) + __attribute__ ((__nothrow__ , __leaf__, __malloc__, __nonnull__ (1))); + +extern void abort (void) + __attribute__ ((__nothrow__ , __leaf__, __noreturn__)); + +extern int getopt (int ___argc, char *const *___argv, const char *__shortopts) + __attribute__ ((__nothrow__ , __leaf__, __nonnull__ (2, 3))); +extern char *optarg; + +extern void free (void *__ptr) + __attribute__ ((__nothrow__ , __leaf__)); + +struct state { + const char *confpath; + const char *host; + const char *port; + const char *state_dir_prefix; +}; + +static inline char *xstrdup(const char *s) { + char *val = strdup(s); + if (!val) + abort(); + return val; +} + +int config_init(struct state *config); + +int main(int argc, char *argv[]) { + struct state state; + + config_init(&state); + + switch (getopt(argc, argv, "H:p:")) { + case 'H': + state.host = xstrdup(optarg); + break; + case 'p': + state.port = xstrdup(optarg); + break; + } + + free((void*)state.host); + free((void*)state.port); + return 0; +}