From 3ef328c293a336df0aead2d72c0c5ed9781a9861 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Wed, 2 Feb 2022 16:39:12 -0500 Subject: [PATCH] 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 --- 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(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr104369-1.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr104369-2.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/realloc-3.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/realloc-4.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-realloc.c 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 +#include +#include + +/* 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" } */ +}