From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2209) id C97273858023; Mon, 29 Nov 2021 23:51:56 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C97273858023 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: David Malcolm To: gcc-cvs@gcc.gnu.org Subject: [gcc r12-5585] analyzer: further false leak fixes due to overzealous state merging [PR103217] X-Act-Checkin: gcc X-Git-Author: David Malcolm X-Git-Refname: refs/heads/master X-Git-Oldrev: ca5667e867252db3c8642ee90f55427149cd92b6 X-Git-Newrev: 132902177138c09803d639e12b1daebf2b9edddc Message-Id: <20211129235156.C97273858023@sourceware.org> Date: Mon, 29 Nov 2021 23:51:56 +0000 (GMT) X-BeenThere: gcc-cvs@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Nov 2021 23:51:56 -0000 https://gcc.gnu.org/g:132902177138c09803d639e12b1daebf2b9edddc commit r12-5585-g132902177138c09803d639e12b1daebf2b9edddc Author: David Malcolm Date: Mon Nov 29 11:47:47 2021 -0500 analyzer: further false leak fixes due to overzealous state merging [PR103217] 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". 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 Diff: --- 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(-) 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; +}