diff mbox series

[committed,1/2] analyzer: treat pointers written to *UNKNOWN as escaping [PR98575]

Message ID 20210209205751.1335450-1-dmalcolm@redhat.com
State New
Headers show
Series [committed,1/2] analyzer: treat pointers written to *UNKNOWN as escaping [PR98575] | expand

Commit Message

David Malcolm Feb. 9, 2021, 8:57 p.m. UTC 
PR analyzer/98575 describes an unexpected -Wanalyzer-malloc-leak false
positive from gcc.dg/analyzer/pr94851-1.c on glibc < 2.28.

The issue is that a getchar call gets inlined into a call to _IO_getc,
and "_IO_getc" is not in the set of FILE * functions the analyzer
"knows about".  This leads to a global pointer
  struct buf *curbp;
being treated as UNKNOWN after the call to _IO_getc.  Later when a
malloced pointer is written to curbp->b_amark, the write is discarded
(since curbp is unknown) without noting that the pointer has escaped,
and so the pointer is erroneously treated as leaking when the function
returns.

This patch updates the handling of *UNKNOWN to treat pointers written
to them as having escaped, fixing the false positive.

The patch stops the leak warning in gcc.dg/analyzer/explode-1.c.
After merging states at the join-point after the first switch, pp has
UNKNOWN value, and so *pp is a write through UNKNOWN, which with this
patch is now treated as escaping - despite the fact that all possible
values for *pp are on the stack.  There doesn't seem to be a good way
to fix this, and the testcase is an artifically constructed one, so the
patch simply removes the dg-warning directive.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Verified the fix on gcc135.fsffrance.org.
Pushed to trunk as r11-7155-g1d9f3b7ad4f965a0acc21d42cb2d186ecd065b71.

gcc/analyzer/ChangeLog:
	PR analyzer/98575
	* store.cc (store::set_value): Treat a pointer written to *UNKNOWN
	as having escaped.

gcc/testsuite/ChangeLog:
	PR analyzer/98575
	* gcc.dg/analyzer/explode-1.c: Remove expected leak warning.
	* gcc.dg/analyzer/pr94851-2.c: New test.
	* gcc.dg/analyzer/pr98575-1.c: New test.
---
 gcc/analyzer/store.cc                     | 17 +++++--
 gcc/testsuite/gcc.dg/analyzer/explode-1.c |  2 +-
 gcc/testsuite/gcc.dg/analyzer/pr94851-2.c | 54 +++++++++++++++++++++++
 gcc/testsuite/gcc.dg/analyzer/pr98575-1.c | 46 +++++++++++++++++++
 4 files changed, 115 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr94851-2.c
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr98575-1.c
diff mbox series

Patch

diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc
index abdb336da91..da5b5adb5b4 100644
--- a/gcc/analyzer/store.cc
+++ b/gcc/analyzer/store.cc
@@ -1820,9 +1820,20 @@  store::set_value (store_manager *mgr, const region *lhs_reg,
   const region *lhs_base_reg = lhs_reg->get_base_region ();
   binding_cluster *lhs_cluster;
   if (lhs_base_reg->symbolic_for_unknown_ptr_p ())
-    /* Reject attempting to bind values into a symbolic region
-       for an unknown ptr; merely invalidate values below.  */
-    lhs_cluster = NULL;
+    {
+      /* Reject attempting to bind values into a symbolic region
+	 for an unknown ptr; merely invalidate values below.  */
+      lhs_cluster = NULL;
+
+      /* The LHS of the write is *UNKNOWN.  If the RHS is a pointer,
+	 then treat the region being pointed to as having escaped.  */
+      if (const region_svalue *ptr_sval = rhs_sval->dyn_cast_region_svalue ())
+	{
+	  const region *ptr_dst = ptr_sval->get_pointee ();
+	  const region *ptr_base_reg = ptr_dst->get_base_region ();
+	  mark_as_escaped (ptr_base_reg);
+	}
+    }
   else
     {
       lhs_cluster = get_or_create_cluster (lhs_base_reg);
diff --git a/gcc/testsuite/gcc.dg/analyzer/explode-1.c b/gcc/testsuite/gcc.dg/analyzer/explode-1.c
index 9b95afd9a03..6b62e8e871c 100644
--- a/gcc/testsuite/gcc.dg/analyzer/explode-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/explode-1.c
@@ -47,7 +47,7 @@  void test (void)
 	{
 	default:
 	case 0:
-	  *pp = malloc (16); /* { dg-warning "leak" } */
+	  *pp = malloc (16);
 	  break;
 	case 1:
 	  free (*pp);
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr94851-2.c b/gcc/testsuite/gcc.dg/analyzer/pr94851-2.c
new file mode 100644
index 00000000000..60947216b7f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr94851-2.c
@@ -0,0 +1,54 @@ 
+/* As pr94851-1.c, but verify that we don't get confused by a call to
+   an unknown function (PR analyzer/98575).  */
+
+/* { dg-additional-options "-O2" } */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+typedef struct AMARK {
+  struct AMARK *m_next;
+  char m_name;
+} AMARK;
+
+struct buf {
+  AMARK *b_amark;
+};
+
+struct buf *curbp;
+
+extern void unknown_fn (void);
+
+int pamark(void) {
+  int c;
+
+  AMARK *p = curbp->b_amark;
+  AMARK *last = curbp->b_amark;
+
+  unknown_fn ();
+
+  c = getchar ();
+
+  while (p != (AMARK *)NULL && p->m_name != (char)c) {
+    last = p;
+    p = p->m_next;
+  }
+
+  if (p != (AMARK *)NULL) {
+    printf("over writing mark %c\n", c);
+  } else {
+    if ((p = (AMARK *)malloc(sizeof(AMARK))) == (AMARK *)NULL)
+      return 0;
+
+    p->m_next = (AMARK *)NULL;
+
+    if (curbp->b_amark == (AMARK *)NULL)
+      curbp->b_amark = p;
+    else
+      last->m_next = p;
+  }
+
+  p->m_name = (char)c; /* { dg-bogus "leak of 'p'" "bogus leak" } */
+
+  return 1;
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr98575-1.c b/gcc/testsuite/gcc.dg/analyzer/pr98575-1.c
new file mode 100644
index 00000000000..6472e762f0c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr98575-1.c
@@ -0,0 +1,46 @@ 
+/* A malloced pointer that's written to a global pointer shouldn't be
+   reported as leaking, even if an unknown function has been called
+   (PR analyzer/98575).  */
+
+void **g;
+
+extern void unknown_fn (void);
+
+/* Without a call to unknown_fn.  */
+
+int test_1 (void)
+{
+  void *p;
+  p = __builtin_malloc(1024);
+  *g = p;
+  return 0;
+}
+
+/* With a call to unknown_fn in various places.  */
+
+int test_2 (void)
+{
+  void *p;
+  unknown_fn ();
+  p = __builtin_malloc(1024);
+  *g = p;
+  return 0;
+}
+
+int test_3 (void)
+{
+  void *p;
+  p = __builtin_malloc(1024);
+  unknown_fn ();
+  *g = p;
+  return 0;
+}
+
+int test_4 (void)
+{
+  void *p;
+  p = __builtin_malloc(1024);
+  *g = p;
+  unknown_fn ();
+  return 0;
+}