diff mbox series

[1/2,v2] analyzer: consider that realloc could shrink the buffer [PR106539]

Message ID 20220811172452.65996-1-mail@tim-lange.me
State New, archived
Headers
Series [1/2,v2] analyzer: consider that realloc could shrink the buffer [PR106539] |

Commit Message

Tim Lange Aug. 11, 2022, 5:24 p.m. UTC 
  This patch adds the "shrinks buffer" case to the success_with_move
modelling of realloc.

Regression-tested on Linux x86-64, further ran the analyzer tests with
the -m32 option.

2022-08-11  Tim Lange  <mail@tim-lange.me>

gcc/analyzer/ChangeLog:

	PR analyzer/106539
	* region-model-impl-calls.cc (region_model::impl_call_realloc):
	Use the result of get_copied_size as the size for the
	sized_regions in realloc.
	(success_with_move::get_copied_size): New function.

gcc/testsuite/ChangeLog:

	PR analyzer/106539
	* gcc.dg/analyzer/pr106539.c: New test.
	* gcc.dg/analyzer/realloc-5.c: New test.

---
 gcc/analyzer/region-model-impl-calls.cc   | 48 ++++++++++++++++++++---
 gcc/testsuite/gcc.dg/analyzer/pr106539.c  | 15 +++++++
 gcc/testsuite/gcc.dg/analyzer/realloc-5.c | 45 +++++++++++++++++++++
 3 files changed, 102 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr106539.c
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/realloc-5.c

Comments

David Malcolm Aug. 11, 2022, 7:25 p.m. UTC | #1 
On Thu, 2022-08-11 at 19:24 +0200, Tim Lange wrote:
> This patch adds the "shrinks buffer" case to the success_with_move
> modelling of realloc.

Thanks for the updated patch.

> 
> Regression-tested on Linux x86-64, further ran the analyzer tests
> with
> the -m32 option.

[...snip...]

> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/realloc-5.c
> @@ -0,0 +1,45 @@
> +#include "analyzer-decls.h"
> +
> +typedef __SIZE_TYPE__ size_t;
> +
> +#define NULL ((void *)0)
> +
> +extern void *malloc (size_t __size)
> +  __attribute__ ((__nothrow__ , __leaf__))
> +  __attribute__ ((__malloc__))
> +  __attribute__ ((__alloc_size__ (1)));
> +extern void *realloc (void *__ptr, size_t __size)
> +  __attribute__ ((__nothrow__ , __leaf__))
> +  __attribute__ ((__warn_unused_result__))
> +  __attribute__ ((__alloc_size__ (2)));
> +extern void free (void *__ptr)
> +  __attribute__ ((__nothrow__ , __leaf__));
> +extern void *memset (void *__ptr, int __value, size_t __size);
> +
> +/* realloc where the region shrinks on success_with_move.  */
> +
> +void test_1 ()
> +{
> +  char *p = malloc (16);
> +  if (!p)
> +    return;
> +  memset (p, 1, 16);
> +
> +  char *q = realloc (p, 8);
> +  if (!q)
> +    {
> +      free (p);
> +      return;
> +    }
> +  else if (p != q)
> +    {
> +      __analyzer_dump_capacity (q); /* { dg-warning "capacity:
> '\\(\[^\n\r\]*\\)8'" } */
> +      __analyzer_eval (q[8] == 1); /* { dg-line eval } */
> +    
> +      /* { dg-warning "UNKNOWN" "warning" { target *-*-* } eval } */
> +      /* { dg-warning "overread" "warning" { target *-*-* } eval }
> */

Strictly speaking, this "overread" warning is dependent on the followup
patch, but assuming that that patch is OK, this patch is also OK.

Thanks
Dave

> +      /* { dg-warning "use of uninitialized value" "warning" {
> target *-*-* } eval } */
> +    }
> +
> +  free (q);
> +}
diff mbox series

Patch

diff --git a/gcc/analyzer/region-model-impl-calls.cc b/gcc/analyzer/region-model-impl-calls.cc
index 8c38e9206fa..fa0ec88b1f4 100644
--- a/gcc/analyzer/region-model-impl-calls.cc
+++ b/gcc/analyzer/region-model-impl-calls.cc
@@ -732,15 +732,17 @@  region_model::impl_call_realloc (const call_details &cd)
 	  const svalue *old_size_sval = model->get_dynamic_extents (freed_reg);
 	  if (old_size_sval)
 	    {
-	      const region *sized_old_reg
+	      const svalue *copied_size_sval
+		= get_copied_size (old_size_sval, new_size_sval);
+	      const region *copied_old_reg
 		= model->m_mgr->get_sized_region (freed_reg, NULL,
-						  old_size_sval);
+						  copied_size_sval);
 	      const svalue *buffer_content_sval
-		= model->get_store_value (sized_old_reg, cd.get_ctxt ());
-	      const region *sized_new_reg
+		= model->get_store_value (copied_old_reg, cd.get_ctxt ());
+	      const region *copied_new_reg
 		= model->m_mgr->get_sized_region (new_reg, NULL,
-						  old_size_sval);
-	      model->set_value (sized_new_reg, buffer_content_sval,
+						  copied_size_sval);
+	      model->set_value (copied_new_reg, buffer_content_sval,
 				cd.get_ctxt ());
 	    }
 	  else
@@ -774,6 +776,40 @@  region_model::impl_call_realloc (const call_details &cd)
       else
 	return true;
     }
+
+  private:
+    /* Return the lesser of OLD_SIZE_SVAL and NEW_SIZE_SVAL.
+       If either one is symbolic, the symbolic svalue is returned.  */
+    const svalue *get_copied_size (const svalue *old_size_sval,
+				   const svalue *new_size_sval) const
+    {
+      tree old_size_cst = old_size_sval->maybe_get_constant ();
+      tree new_size_cst = new_size_sval->maybe_get_constant ();
+
+      if (old_size_cst && new_size_cst)
+	{
+	  /* Both are constants and comparable.  */
+	  tree cmp = fold_binary (LT_EXPR, boolean_type_node,
+				  old_size_cst, new_size_cst);
+
+	  if (cmp == boolean_true_node)
+	    return old_size_sval;
+	  else
+	    return new_size_sval;
+	}
+      else if (new_size_cst)
+	{
+	  /* OLD_SIZE_SVAL is symbolic, so return that.  */
+	  return old_size_sval;
+	}
+      else
+	{
+	  /* NEW_SIZE_SVAL is symbolic or both are symbolic.
+	     Return NEW_SIZE_SVAL, because implementations of realloc
+	     probably only moves the buffer if the new size is larger.  */
+	  return new_size_sval;
+	}
+    }
   };
 
   /* Body of region_model::impl_call_realloc.  */
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr106539.c b/gcc/testsuite/gcc.dg/analyzer/pr106539.c
new file mode 100644
index 00000000000..fd270868e36
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr106539.c
@@ -0,0 +1,15 @@ 
+#include <stdlib.h>
+
+void *test (void)
+{
+  void **p = (void **)malloc (sizeof (void *) * 2);
+  if (!p)
+    return NULL;
+  p[0] = malloc(10);
+  p[1] = malloc(20); /* { dg-message "allocated here" }  */
+  void *q = realloc (p, sizeof (void *)); /* { dg-message "when 'realloc' succeeds, moving buffer" } */
+  if (!q)
+  /* { dg-warning "leak of '<unknown>'" "leak of unknown" { target *-*-* } .-1 } */
+    return p;
+  return q;
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/realloc-5.c b/gcc/testsuite/gcc.dg/analyzer/realloc-5.c
new file mode 100644
index 00000000000..2efe3371e12
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/realloc-5.c
@@ -0,0 +1,45 @@ 
+#include "analyzer-decls.h"
+
+typedef __SIZE_TYPE__ size_t;
+
+#define NULL ((void *)0)
+
+extern void *malloc (size_t __size)
+  __attribute__ ((__nothrow__ , __leaf__))
+  __attribute__ ((__malloc__))
+  __attribute__ ((__alloc_size__ (1)));
+extern void *realloc (void *__ptr, size_t __size)
+  __attribute__ ((__nothrow__ , __leaf__))
+  __attribute__ ((__warn_unused_result__))
+  __attribute__ ((__alloc_size__ (2)));
+extern void free (void *__ptr)
+  __attribute__ ((__nothrow__ , __leaf__));
+extern void *memset (void *__ptr, int __value, size_t __size);
+
+/* realloc where the region shrinks on success_with_move.  */
+
+void test_1 ()
+{
+  char *p = malloc (16);
+  if (!p)
+    return;
+  memset (p, 1, 16);
+
+  char *q = realloc (p, 8);
+  if (!q)
+    {
+      free (p);
+      return;
+    }
+  else if (p != q)
+    {
+      __analyzer_dump_capacity (q); /* { dg-warning "capacity: '\\(\[^\n\r\]*\\)8'" } */
+      __analyzer_eval (q[8] == 1); /* { dg-line eval } */
+    
+      /* { dg-warning "UNKNOWN" "warning" { target *-*-* } eval } */
+      /* { dg-warning "overread" "warning" { target *-*-* } eval } */
+      /* { dg-warning "use of uninitialized value" "warning" { target *-*-* } eval } */
+    }
+
+  free (q);
+}