[gcc r12-7040] analyzer: fixes to realloc-handling [PR104369]
David Malcolm
dmalcolm@gcc.gnu.org
Thu Feb 3 22:48:25 GMT 2022
https://gcc.gnu.org/g:3ef328c293a336df0aead2d72c0c5ed9781a9861
commit r12-7040-g3ef328c293a336df0aead2d72c0c5ed9781a9861
Author: David Malcolm <dmalcolm@redhat.com>
Date: Wed Feb 2 16:39:12 2022 -0500
analyzer: fixes to realloc-handling [PR104369]
This patch fixes various issues with how -fanalyzer handles "realloc"
seen when debugging PR analyzer/104369.
Previously it wasn't correctly copying over the contents of the old
buffer for the success-with-move case, leading to false
-Wanalyzer-use-of-uninitialized-value diagnostics.
I also noticed that -fanalyzer failed to properly handle "realloc" for
cases where the ptr's region had unknown dynamic extents, and an ICE
for the case where a tainted value is used as a realloc size argument.
This patch fixes these issues, including the false uninit diagnostics
seen in PR analyzer/104369.
gcc/analyzer/ChangeLog:
PR analyzer/104369
* engine.cc (exploded_graph::process_node): Use the node for any
diagnostics, avoiding ICE if a bifurcation update adds a
saved_diagnostic, such as for a tainted realloc size.
* region-model-impl-calls.cc
(region_model::impl_call_realloc::success_no_move::update_model):
Require the old pointer to be non-NULL to be able successfully
grow in place. Use model->deref_rvalue rather than maybe_get_region
to support the old pointer being symbolic.
(region_model::impl_call_realloc::success_with_move::update_model):
Likewise. Add a constraint that the new pointer != the old pointer.
Use a sized_region when setting the value of the new region.
Handle the case where we don't know the dynamic size of the old
region by marking the new region as unknown.
* sm-taint.cc (tainted_allocation_size::tainted_allocation_size):
Update assertion to also allow for MEMSPACE_UNKNOWN.
(tainted_allocation_size::emit): Likewise.
(region_model::check_dynamic_size_for_taint): Likewise.
gcc/testsuite/ChangeLog:
PR analyzer/104369
* gcc.dg/analyzer/pr104369-1.c: New test.
* gcc.dg/analyzer/pr104369-2.c: New test.
* gcc.dg/analyzer/realloc-3.c: New test.
* gcc.dg/analyzer/realloc-4.c: New test.
* gcc.dg/analyzer/taint-realloc.c: New test.
Signed-off-by: David Malcolm <dmalcolm@redhat.com>
Diff:
---
gcc/analyzer/engine.cc | 2 +-
gcc/analyzer/region-model-impl-calls.cc | 33 +++++++++-
gcc/analyzer/sm-taint.cc | 12 +++-
gcc/testsuite/gcc.dg/analyzer/pr104369-1.c | 86 +++++++++++++++++++++++++++
gcc/testsuite/gcc.dg/analyzer/pr104369-2.c | 79 ++++++++++++++++++++++++
gcc/testsuite/gcc.dg/analyzer/realloc-3.c | 81 +++++++++++++++++++++++++
gcc/testsuite/gcc.dg/analyzer/realloc-4.c | 85 ++++++++++++++++++++++++++
gcc/testsuite/gcc.dg/analyzer/taint-realloc.c | 21 +++++++
8 files changed, 392 insertions(+), 7 deletions(-)
diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index 243235e4cd4..bff37798a39 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -3799,7 +3799,7 @@ exploded_graph::process_node (exploded_node *node)
/* Apply edge_info to state. */
impl_region_model_context
bifurcation_ctxt (*this,
- NULL, // enode_for_diag
+ node, // enode_for_diag
&path_ctxt.get_state_at_bifurcation (),
&bifurcated_new_state,
NULL, // uncertainty_t *uncertainty
diff --git a/gcc/analyzer/region-model-impl-calls.cc b/gcc/analyzer/region-model-impl-calls.cc
index 779d94388e9..e9592975332 100644
--- a/gcc/analyzer/region-model-impl-calls.cc
+++ b/gcc/analyzer/region-model-impl-calls.cc
@@ -651,7 +651,18 @@ region_model::impl_call_realloc (const call_details &cd)
const call_details cd (get_call_details (model, ctxt));
const svalue *ptr_sval = cd.get_arg_svalue (0);
const svalue *size_sval = cd.get_arg_svalue (1);
- if (const region *buffer_reg = ptr_sval->maybe_get_region ())
+
+ /* We can only grow in place with a non-NULL pointer. */
+ {
+ const svalue *null_ptr
+ = model->m_mgr->get_or_create_int_cst (ptr_sval->get_type (), 0);
+ if (!model->add_constraint (ptr_sval, NE_EXPR, null_ptr,
+ cd.get_ctxt ()))
+ return false;
+ }
+
+ if (const region *buffer_reg = model->deref_rvalue (ptr_sval, NULL_TREE,
+ ctxt))
if (compat_types_p (size_sval->get_type (), size_type_node))
model->set_dynamic_extents (buffer_reg, size_sval, ctxt);
if (cd.get_lhs_region ())
@@ -696,10 +707,15 @@ region_model::impl_call_realloc (const call_details &cd)
= model->create_region_for_heap_alloc (new_size_sval, ctxt);
const svalue *new_ptr_sval
= model->m_mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
+ if (!model->add_constraint (new_ptr_sval, NE_EXPR, old_ptr_sval,
+ cd.get_ctxt ()))
+ return false;
+
if (cd.get_lhs_type ())
cd.maybe_set_lhs (new_ptr_sval);
- if (const region *freed_reg = old_ptr_sval->maybe_get_region ())
+ if (const region *freed_reg = model->deref_rvalue (old_ptr_sval,
+ NULL_TREE, ctxt))
{
/* Copy the data. */
const svalue *old_size_sval = model->get_dynamic_extents (freed_reg);
@@ -710,7 +726,18 @@ region_model::impl_call_realloc (const call_details &cd)
old_size_sval);
const svalue *buffer_content_sval
= model->get_store_value (sized_old_reg, cd.get_ctxt ());
- model->set_value (new_reg, buffer_content_sval, cd.get_ctxt ());
+ const region *sized_new_reg
+ = model->m_mgr->get_sized_region (new_reg, NULL,
+ old_size_sval);
+ model->set_value (sized_new_reg, buffer_content_sval,
+ cd.get_ctxt ());
+ }
+ else
+ {
+ /* We don't know how big the old region was;
+ mark the new region as having been touched to avoid uninit
+ issues. */
+ model->mark_region_as_unknown (new_reg, cd.get_uncertainty ());
}
/* Free the old region, so that pointers to the old buffer become
diff --git a/gcc/analyzer/sm-taint.cc b/gcc/analyzer/sm-taint.cc
index 357456593ff..573a9124286 100644
--- a/gcc/analyzer/sm-taint.cc
+++ b/gcc/analyzer/sm-taint.cc
@@ -496,7 +496,9 @@ public:
: taint_diagnostic (sm, arg, has_bounds),
m_mem_space (mem_space)
{
- gcc_assert (mem_space == MEMSPACE_STACK || mem_space == MEMSPACE_HEAP);
+ gcc_assert (mem_space == MEMSPACE_STACK
+ || mem_space == MEMSPACE_HEAP
+ || mem_space == MEMSPACE_UNKNOWN);
}
const char *get_kind () const FINAL OVERRIDE
@@ -509,7 +511,9 @@ public:
diagnostic_metadata m;
/* "CWE-789: Memory Allocation with Excessive Size Value". */
m.add_cwe (789);
- gcc_assert (m_mem_space == MEMSPACE_STACK || m_mem_space == MEMSPACE_HEAP);
+ gcc_assert (m_mem_space == MEMSPACE_STACK
+ || m_mem_space == MEMSPACE_HEAP
+ || m_mem_space == MEMSPACE_UNKNOWN);
// TODO: make use of m_mem_space
if (m_arg)
switch (m_has_bounds)
@@ -1051,7 +1055,9 @@ region_model::check_dynamic_size_for_taint (enum memory_space mem_space,
const svalue *size_in_bytes,
region_model_context *ctxt) const
{
- gcc_assert (mem_space == MEMSPACE_STACK || mem_space == MEMSPACE_HEAP);
+ gcc_assert (mem_space == MEMSPACE_STACK
+ || mem_space == MEMSPACE_HEAP
+ || mem_space == MEMSPACE_UNKNOWN);
gcc_assert (size_in_bytes);
gcc_assert (ctxt);
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr104369-1.c b/gcc/testsuite/gcc.dg/analyzer/pr104369-1.c
new file mode 100644
index 00000000000..d3b32418a9e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr104369-1.c
@@ -0,0 +1,86 @@
+/* { dg-additional-options "--param analyzer-max-enodes-per-program-point=10" } */
+// TODO: remove need for this option
+
+typedef __SIZE_TYPE__ size_t;
+#define NULL ((void *)0)
+#define POLLIN 0x001
+
+typedef struct {
+ unsigned long int __val[(1024 / (8 * sizeof(unsigned long int)))];
+} __sigset_t;
+
+typedef unsigned int socklen_t;
+
+struct timespec {
+ /* [...snip...] */
+};
+
+typedef unsigned long int nfds_t;
+
+struct pollfd {
+ int fd;
+ short int events;
+ short int revents;
+};
+
+extern int ppoll(struct pollfd *__fds, nfds_t __nfds,
+ const struct timespec *__timeout, const __sigset_t *__ss);
+
+struct sockaddr_un {
+ /* [...snip...] */
+ char sun_path[108];
+};
+
+typedef union {
+ /* [...snip...] */
+ struct sockaddr_un *__restrict __sockaddr_un__;
+ /* [...snip...] */
+} __SOCKADDR_ARG __attribute__((transparent_union));
+
+extern int accept(int __fd, __SOCKADDR_ARG __addr,
+ socklen_t *__restrict __addr_len);
+
+extern void *calloc(size_t __nmemb, size_t __size)
+ __attribute__((__nothrow__, __leaf__))
+ __attribute__((__malloc__))
+ __attribute__((__alloc_size__(1, 2)));
+
+extern void *realloc(void *__ptr, size_t __size)
+ __attribute__((__nothrow__, __leaf__))
+ __attribute__((__warn_unused_result__))
+ __attribute__((__alloc_size__(2)));
+
+extern void exit(int __status)
+ __attribute__((__nothrow__, __leaf__))
+ __attribute__((__noreturn__));
+
+int main() {
+ int rc;
+ int nsockets = 1;
+ struct pollfd *pollfds, *newpollfds;
+ struct sockaddr_un remote;
+ socklen_t len = sizeof(remote);
+
+ pollfds = calloc(1, sizeof(struct pollfd));
+ if (!pollfds) {
+ exit(1);
+ }
+
+ while (1) {
+ rc = ppoll(pollfds, nsockets, NULL, NULL);
+ if (rc < 0) {
+ continue;
+ }
+
+ if (pollfds[0].revents & POLLIN) {
+ nsockets++;
+ newpollfds = realloc(pollfds, nsockets * sizeof(*pollfds));
+ if (!newpollfds) {
+ exit(1);
+ }
+ pollfds = newpollfds;
+ pollfds[nsockets - 1].fd = accept(pollfds[0].fd, &remote, &len);
+ }
+ }
+ return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr104369-2.c b/gcc/testsuite/gcc.dg/analyzer/pr104369-2.c
new file mode 100644
index 00000000000..57dc9caf3e9
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr104369-2.c
@@ -0,0 +1,79 @@
+typedef __SIZE_TYPE__ size_t;
+#define NULL ((void *)0)
+#define POLLIN 0x001
+
+typedef struct {
+ unsigned long int __val[(1024 / (8 * sizeof(unsigned long int)))];
+} __sigset_t;
+
+typedef unsigned int socklen_t;
+
+struct timespec {
+ /* [...snip...] */
+};
+
+typedef unsigned long int nfds_t;
+
+struct pollfd {
+ int fd;
+ short int events;
+ short int revents;
+};
+
+extern int ppoll(struct pollfd *__fds, nfds_t __nfds,
+ const struct timespec *__timeout, const __sigset_t *__ss);
+
+struct sockaddr_un {
+ /* [...snip...] */
+ char sun_path[108];
+};
+
+typedef union {
+ /* [...snip...] */
+ struct sockaddr_un *__restrict __sockaddr_un__;
+ /* [...snip...] */
+} __SOCKADDR_ARG __attribute__((transparent_union));
+
+extern int accept(int __fd, __SOCKADDR_ARG __addr,
+ socklen_t *__restrict __addr_len);
+
+extern void *calloc(size_t __nmemb, size_t __size)
+ __attribute__((__nothrow__, __leaf__))
+ __attribute__((__malloc__))
+ __attribute__((__alloc_size__(1, 2)));
+
+extern void *realloc(void *__ptr, size_t __size)
+ __attribute__((__nothrow__, __leaf__))
+ __attribute__((__warn_unused_result__))
+ __attribute__((__alloc_size__(2)));
+
+extern void exit(int __status)
+ __attribute__((__nothrow__, __leaf__))
+ __attribute__((__noreturn__));
+
+int main() {
+ int rc;
+ int nsockets = 1;
+ struct pollfd *pollfds, *newpollfds;
+ struct sockaddr_un remote;
+ socklen_t len = sizeof(remote);
+
+ pollfds = calloc(1, sizeof(struct pollfd));
+ if (!pollfds) {
+ exit(1);
+ }
+
+ rc = ppoll(pollfds, nsockets, NULL, NULL);
+ if (rc < 0) {
+ exit(2);
+ }
+
+ nsockets++;
+ newpollfds = realloc(pollfds, nsockets * sizeof(*pollfds));
+ if (!newpollfds) {
+ exit(3);
+ }
+ pollfds = newpollfds;
+ pollfds[nsockets - 1].fd = accept(pollfds[0].fd, &remote, &len);
+ exit(4);
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/realloc-3.c b/gcc/testsuite/gcc.dg/analyzer/realloc-3.c
new file mode 100644
index 00000000000..89676e1ca0d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/realloc-3.c
@@ -0,0 +1,81 @@
+#include "analyzer-decls.h"
+
+typedef __SIZE_TYPE__ size_t;
+
+#define NULL ((void *)0)
+
+extern void *calloc (size_t __nmemb, size_t __size)
+ __attribute__ ((__nothrow__ , __leaf__))
+ __attribute__ ((__malloc__))
+ __attribute__ ((__alloc_size__ (1, 2))) ;
+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__));
+
+/* realloc of calloc buffer. */
+
+char *test_8 (size_t sz)
+{
+ char *p, *q;
+
+ p = calloc (1, 3);
+ if (!p)
+ return NULL;
+
+ __analyzer_dump_capacity (p); /* { dg-warning "capacity: '\\(\[^\n\r\]*\\)3'" } */
+
+ __analyzer_eval (p[0] == 0); /* { dg-warning "TRUE" } */
+ __analyzer_eval (p[1] == 0); /* { dg-warning "TRUE" } */
+ __analyzer_eval (p[2] == 0); /* { dg-warning "TRUE" } */
+
+ q = realloc (p, 6);
+
+ /* We should have 3 nodes, corresponding to "failure",
+ "success without moving", and "success with moving". */
+ __analyzer_dump_exploded_nodes (0); /* { dg-warning "3 processed enodes" } */
+
+ if (q)
+ {
+ __analyzer_dump_capacity (q); /* { dg-warning "capacity: '\\(\[^\n\r\]*\\)6'" } */
+ q[3] = 'd';
+ q[4] = 'e';
+ q[5] = 'f';
+ if (q == p)
+ {
+ /* "realloc" success, growing the buffer in-place. */
+ __analyzer_eval (p[0] == 0); /* { dg-warning "TRUE" } */
+ __analyzer_eval (p[1] == 0); /* { dg-warning "TRUE" } */
+ __analyzer_eval (p[2] == 0); /* { dg-warning "TRUE" } */
+ }
+ else
+ {
+ /* "realloc" success, moving the buffer (and thus freeing "p"). */
+ __analyzer_eval (q[0] == 0); /* { dg-warning "TRUE" } */
+ __analyzer_eval (q[1] == 0); /* { dg-warning "TRUE" } */
+ __analyzer_eval (q[2] == 0); /* { dg-warning "TRUE" } */
+ __analyzer_eval (p[0] == 'a'); /* { dg-warning "UNKNOWN" "unknown" } */
+ /* { dg-warning "use after 'free' of 'p'" "use after free" { target *-*-* } .-1 } */
+ }
+ __analyzer_eval (q[3] == 'd'); /* { dg-warning "TRUE" } */
+ __analyzer_eval (q[4] == 'e'); /* { dg-warning "TRUE" } */
+ __analyzer_eval (q[5] == 'f'); /* { dg-warning "TRUE" } */
+ }
+ else
+ {
+ /* "realloc" failure. p should be unchanged. */
+ __analyzer_dump_capacity (p); /* { dg-warning "capacity: '\\(\[^\n\r\]*\\)3'" } */
+ __analyzer_eval (p[0] == 0); /* { dg-warning "TRUE" } */
+ __analyzer_eval (p[1] == 0); /* { dg-warning "TRUE" } */
+ __analyzer_eval (p[2] == 0); /* { dg-warning "TRUE" } */
+ return p;
+ }
+
+ return q;
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/realloc-4.c b/gcc/testsuite/gcc.dg/analyzer/realloc-4.c
new file mode 100644
index 00000000000..ac338ec0497
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/realloc-4.c
@@ -0,0 +1,85 @@
+#include "analyzer-decls.h"
+
+typedef __SIZE_TYPE__ size_t;
+
+#define NULL ((void *)0)
+
+extern void *calloc (size_t __nmemb, size_t __size)
+ __attribute__ ((__nothrow__ , __leaf__))
+ __attribute__ ((__malloc__))
+ __attribute__ ((__alloc_size__ (1, 2))) ;
+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__));
+
+/* realloc where we don't know the original size of the region. */
+
+char *test_8 (char *p, size_t sz)
+{
+ char *q;
+
+ __analyzer_dump_capacity (p); /* { dg-warning "capacity: 'UNKNOWN\\(\[^\n\r\]*\\)'" } */
+
+ p[0] = 'a';
+ p[1] = 'b';
+ p[2] = 'c';
+ __analyzer_eval (p[0] == 'a'); /* { dg-warning "TRUE" } */
+ __analyzer_eval (p[1] == 'b'); /* { dg-warning "TRUE" } */
+ __analyzer_eval (p[2] == 'c'); /* { dg-warning "TRUE" } */
+
+ q = realloc (p, 6);
+
+ /* We should have 3 nodes, corresponding to "failure",
+ "success without moving", and "success with moving". */
+ __analyzer_dump_exploded_nodes (0); /* { dg-warning "3 processed enodes" } */
+
+ if (q)
+ {
+ q[3] = 'd';
+ q[4] = 'e';
+ q[5] = 'f';
+ if (q == p)
+ {
+ /* "realloc" success, growing the buffer in-place. */
+ __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */
+ __analyzer_dump_capacity (q); /* { dg-warning "capacity: '\\(\[^\n\r\]*\\)6'" } */
+ __analyzer_eval (q[0] == 'a'); /* { dg-warning "TRUE" } */
+ __analyzer_eval (q[1] == 'b'); /* { dg-warning "TRUE" } */
+ __analyzer_eval (q[2] == 'c'); /* { dg-warning "TRUE" } */
+ }
+ else
+ {
+ /* "realloc" success, moving the buffer (and thus freeing "p"). */
+ __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */
+ __analyzer_dump_capacity (q); /* { dg-warning "capacity: '\\(\[^\n\r\]*\\)6'" } */
+ /* We don't know how much of the buffer is copied. */
+ __analyzer_eval (q[0] == 'a'); /* { dg-warning "UNKNOWN" } */
+ __analyzer_eval (q[1] == 'b'); /* { dg-warning "UNKNOWN" } */
+ __analyzer_eval (q[2] == 'c'); /* { dg-warning "UNKNOWN" } */
+ __analyzer_eval (p[0] == 'a'); /* { dg-warning "UNKNOWN" "unknown" } */
+ /* { dg-warning "use after 'free' of 'p'" "use after free" { target *-*-* } .-1 } */
+ }
+ __analyzer_eval (q[3] == 'd'); /* { dg-warning "TRUE" } */
+ __analyzer_eval (q[4] == 'e'); /* { dg-warning "TRUE" } */
+ __analyzer_eval (q[5] == 'f'); /* { dg-warning "TRUE" } */
+ }
+ else
+ {
+ /* "realloc" failure. p should be unchanged. */
+ __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */
+ __analyzer_dump_capacity (q); /* { dg-warning "capacity: 'UNKNOWN\\(\[^\n\r\]*\\)'" } */
+ __analyzer_eval (p[0] == 'a'); /* { dg-warning "TRUE" } */
+ __analyzer_eval (p[1] == 'b'); /* { dg-warning "TRUE" } */
+ __analyzer_eval (p[2] == 'c'); /* { dg-warning "TRUE" } */
+ return p;
+ }
+
+ return q;
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/taint-realloc.c b/gcc/testsuite/gcc.dg/analyzer/taint-realloc.c
new file mode 100644
index 00000000000..bd0ed0010fb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/taint-realloc.c
@@ -0,0 +1,21 @@
+// TODO: remove need for this option:
+/* { dg-additional-options "-fanalyzer-checker=taint" } */
+
+#include "analyzer-decls.h"
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+/* realloc with tainted size. */
+
+void *p;
+
+void __attribute__((tainted_args))
+test_1 (size_t sz) /* { dg-message "\\(1\\) function 'test_1' marked with '__attribute__\\(\\(tainted_args\\)\\)'" } */
+{
+ void *q;
+
+ __analyzer_dump_state ("taint", sz); /* { dg-warning "state: 'tainted'" } */
+
+ q = realloc (p, sz); /* { dg-warning "use of attacker-controlled value 'sz' as allocation size without upper-bounds checking" } */
+}
More information about the Gcc-cvs
mailing list