diff mbox series

[committed] analyzer: further false leak fixes due to overzealous state merging [PR103217]

Message ID 20211129235330.3127723-1-dmalcolm@redhat.com
State New
Headers show
Series [committed] analyzer: further false leak fixes due to overzealous state merging [PR103217] | expand

Commit Message

David Malcolm Nov. 29, 2021, 11:53 p.m. UTC 
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 <dmalcolm@redhat.com>
---
 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 mbox series

Patch

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<const binding_key *>::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;
+}