* [PATCH 1/2] analyzer: consider that realloc could shrink the buffer [PR106539]
@ 2022-08-09 21:19 Tim Lange
2022-08-09 21:19 ` [PATCH 2/2] analyzer: out-of-bounds checker [PR106000] Tim Lange
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Tim Lange @ 2022-08-09 21:19 UTC (permalink / raw)
To: gcc-patches; +Cc: dmalcolm, Tim Lange
This patch adds the "shrinks buffer" case to the success_with_move
modelling of realloc.
2022-08-09 Tim Lange <mail@tim-lange.me>
gcc/analyzer/ChangeLog:
PR analyzer/106539
* region-model-impl-calls.cc (region_model::impl_call_realloc):
Add get_copied_size function and pass the result as the size of the
new sized_region.
---
gcc/analyzer/region-model-impl-calls.cc | 37 ++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/gcc/analyzer/region-model-impl-calls.cc b/gcc/analyzer/region-model-impl-calls.cc
index 8c38e9206fa..50a19a52a21 100644
--- a/gcc/analyzer/region-model-impl-calls.cc
+++ b/gcc/analyzer/region-model-impl-calls.cc
@@ -737,9 +737,11 @@ 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 ());
+ const svalue *copied_size_sval
+ = get_copied_size (old_size_sval, new_size_sval);
const region *sized_new_reg
= model->m_mgr->get_sized_region (new_reg, NULL,
- old_size_sval);
+ copied_size_sval);
model->set_value (sized_new_reg, buffer_content_sval,
cd.get_ctxt ());
}
@@ -774,6 +776,39 @@ region_model::impl_call_realloc (const call_details &cd)
else
return true;
}
+
+ private:
+ /* Return the size svalue for the new region allocated by realloc. */
+ 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. */
--
2.37.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 2/2] analyzer: out-of-bounds checker [PR106000] 2022-08-09 21:19 [PATCH 1/2] analyzer: consider that realloc could shrink the buffer [PR106539] Tim Lange @ 2022-08-09 21:19 ` Tim Lange 2022-08-09 22:44 ` David Malcolm 2022-08-09 22:02 ` [PATCH 1/2] analyzer: consider that realloc could shrink the buffer [PR106539] David Malcolm 2022-08-11 17:24 ` [PATCH 1/2 v2] " Tim Lange 2 siblings, 1 reply; 8+ messages in thread From: Tim Lange @ 2022-08-09 21:19 UTC (permalink / raw) To: gcc-patches; +Cc: dmalcolm, Tim Lange This patch adds an experimental out-of-bounds checker to the analyzer. The checker was tested on coreutils, curl, httpd and openssh. It is mostly accurate but does produce false-positives on yacc-generated files and sometimes when the analyzer misses an invariant. These cases will be documented in bugzilla. (Regrtests still running with the latest changes, will report back later.) 2022-08-09 Tim Lange <mail@tim-lange.me> gcc/analyzer/ChangeLog: PR analyzer/106000 * analyzer.opt: Add Wanalyzer-out-of-bounds. * region-model.cc (class out_of_bounds): Diagnostics base class for all out-of-bounds diagnostics. (class past_the_end): Base class derived from out_of_bounds for the buffer_overflow and buffer_overread diagnostics. (class buffer_overflow): Buffer overflow diagnostics. (class buffer_overread): Buffer overread diagnostics. (class buffer_underflow): Buffer underflow diagnostics. (class buffer_underread): Buffer overread diagnostics. (region_model::check_region_bounds): New function to check region bounds for out-of-bounds accesses. (region_model::check_region_access): Add call to check_region_bounds. (region_model::get_representative_tree): New function that accepts a region instead of an svalue. * region-model.h (class region_model): Add region_model::check_region_bounds. * region.cc (region::symbolic_p): New predicate. (offset_region::get_byte_size_sval): Only return the remaining byte size on offset_regions. * region.h: Add region::symbolic_p. * store.cc (byte_range::intersects_p): Add new function equivalent to bit_range::intersects_p. (byte_range::exceeds_p): New function. (byte_range::falls_short_of_p): New function. * store.h (struct byte_range): Add byte_range::intersects_p, byte_range::exceeds_p and byte_range::falls_short_of_p. gcc/ChangeLog: PR analyzer/106000 * doc/invoke.texi: Add Wanalyzer-out-of-bounds. gcc/testsuite/ChangeLog: PR analyzer/106000 * gcc.dg/analyzer/allocation-size-3.c: Disable out-of-bounds warning. * gcc.dg/analyzer/memcpy-2.c: Disable out-of-bounds warning. * gcc.dg/analyzer/pr101962.c: Add dg-warning. * gcc.dg/analyzer/pr97029.c: Add dummy buffer to prevent an out-of-bounds warning. * gcc.dg/analyzer/test-setjmp.h: Add dummy buffer to prevent an out-of-bounds warning. * gcc.dg/analyzer/zlib-3.c: Add dg-bogus. * gcc.dg/analyzer/out-of-bounds-1.c: New test. * gcc.dg/analyzer/out-of-bounds-2.c: New test. * gcc.dg/analyzer/out-of-bounds-3.c: New test. * gcc.dg/analyzer/out-of-bounds-container_of.c: New test. * gcc.dg/analyzer/out-of-bounds-coreutils.c: New test. * gcc.dg/analyzer/out-of-bounds-curl.c: New test. --- gcc/analyzer/analyzer.opt | 4 + gcc/analyzer/region-model.cc | 410 ++++++++++++++++++ gcc/analyzer/region-model.h | 3 + gcc/analyzer/region.cc | 32 ++ gcc/analyzer/region.h | 4 + gcc/analyzer/store.cc | 67 +++ gcc/analyzer/store.h | 9 + gcc/doc/invoke.texi | 12 + .../gcc.dg/analyzer/allocation-size-3.c | 2 + gcc/testsuite/gcc.dg/analyzer/memcpy-2.c | 2 +- .../gcc.dg/analyzer/out-of-bounds-1.c | 119 +++++ .../gcc.dg/analyzer/out-of-bounds-2.c | 83 ++++ .../gcc.dg/analyzer/out-of-bounds-3.c | 91 ++++ .../analyzer/out-of-bounds-container_of.c | 51 +++ .../gcc.dg/analyzer/out-of-bounds-coreutils.c | 29 ++ .../gcc.dg/analyzer/out-of-bounds-curl.c | 41 ++ gcc/testsuite/gcc.dg/analyzer/pr101962.c | 5 +- gcc/testsuite/gcc.dg/analyzer/pr97029.c | 4 +- gcc/testsuite/gcc.dg/analyzer/test-setjmp.h | 4 +- gcc/testsuite/gcc.dg/analyzer/zlib-3.c | 4 +- 20 files changed, 970 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-3.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-container_of.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-coreutils.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-curl.c diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt index 5021376b6fb..8e73af60ceb 100644 --- a/gcc/analyzer/analyzer.opt +++ b/gcc/analyzer/analyzer.opt @@ -158,6 +158,10 @@ Wanalyzer-tainted-size Common Var(warn_analyzer_tainted_size) Init(1) Warning Warn about code paths in which an unsanitized value is used as a size. +Wanalyzer-out-of-bounds +Common Var(warn_analyzer_out_of_bounds) Init(1) Warning +Warn about code paths in which a write or read to a buffer is out-of-bounds. + Wanalyzer-use-after-free Common Var(warn_analyzer_use_after_free) Init(1) Warning Warn about code paths in which a freed value is used. diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index f7df2fca245..2f9382ed96c 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -1268,6 +1268,402 @@ region_model::on_stmt_pre (const gimple *stmt, } } +/* Abstract base class for all out-of-bounds warnings. */ + +class out_of_bounds : public pending_diagnostic_subclass<out_of_bounds> +{ +public: + out_of_bounds (const region *reg, tree diag_arg, byte_range range) + : m_reg (reg), m_diag_arg (diag_arg), m_range (range) + {} + + const char *get_kind () const final override + { + return "out_of_bounds_diagnostic"; + } + + bool operator== (const out_of_bounds &other) const + { + return m_reg == other.m_reg + && m_range == other.m_range + && pending_diagnostic::same_tree_p (m_diag_arg, other.m_diag_arg); + } + + int get_controlling_option () const final override + { + return OPT_Wanalyzer_out_of_bounds; + } + + void mark_interesting_stuff (interesting_t *interest) final override + { + interest->add_region_creation (m_reg); + } + +protected: + const region *m_reg; + tree m_diag_arg; + byte_range m_range; +}; + +/* Abstract subclass to complaing about out-of-bounds + past the end of the buffer. */ + +class past_the_end : public out_of_bounds +{ +public: + past_the_end (const region *reg, tree diag_arg, byte_range range, + tree byte_bound) + : out_of_bounds (reg, diag_arg, range), m_byte_bound (byte_bound) + {} + + bool operator== (const past_the_end &other) const + { + return m_reg == other.m_reg + && m_range == other.m_range + && pending_diagnostic::same_tree_p (m_diag_arg, other.m_diag_arg) + && pending_diagnostic::same_tree_p (m_byte_bound, + other.m_byte_bound); + } + + label_text + describe_region_creation_event (const evdesc::region_creation &ev) final + override + { + if (m_byte_bound && TREE_CODE (m_byte_bound) == INTEGER_CST) + return ev.formatted_print ("capacity is %E bytes", m_byte_bound); + + return label_text (); + } + +protected: + tree m_byte_bound; +}; + +/* Concrete subclass to complain about buffer overflows. */ + +class buffer_overflow : public past_the_end +{ +public: + buffer_overflow (const region *reg, tree diag_arg, + byte_range range, tree byte_bound) + : past_the_end (reg, diag_arg, range, byte_bound) + {} + + bool emit (rich_location *rich_loc) final override + { + diagnostic_metadata m; + bool warned; + switch (m_reg->get_memory_space ()) + { + default: + m.add_cwe (787); + warned = warning_meta (rich_loc, m, get_controlling_option (), + "buffer overflow"); + break; + case MEMSPACE_STACK: + m.add_cwe (121); + warned = warning_meta (rich_loc, m, get_controlling_option (), + "stack-based buffer overflow"); + break; + case MEMSPACE_HEAP: + m.add_cwe (122); + warned = warning_meta (rich_loc, m, get_controlling_option (), + "heap-based buffer overflow"); + break; + } + + if (warned) + { + char num_bytes_past_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (m_range.m_size_in_bytes, num_bytes_past_buf, UNSIGNED); + if (m_diag_arg) + inform (rich_loc->get_loc (), "write is %s bytes past the end" + " of %qE", num_bytes_past_buf, + m_diag_arg); + else + inform (rich_loc->get_loc (), "write is %s bytes past the end" + "of the region", + num_bytes_past_buf); + } + + return warned; + } + + label_text describe_final_event (const evdesc::final_event &ev) + final override + { + byte_size_t start = m_range.get_start_byte_offset (); + byte_size_t end = m_range.get_last_byte_offset (); + char start_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (start, start_buf, SIGNED); + char end_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (end, end_buf, SIGNED); + + if (start == end) + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds write at byte %s but %qE" + " ends at byte %E", start_buf, m_diag_arg, + m_byte_bound); + return ev.formatted_print ("out-of-bounds write at byte %s but region" + " ends at byte %E", start_buf, + m_byte_bound); + } + else + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds write from byte %s till" + " byte %s but %qE ends at byte %E", + start_buf, end_buf, m_diag_arg, + m_byte_bound); + return ev.formatted_print ("out-of-bounds write from byte %s till" + " byte %s but region ends at byte %E", + start_buf, end_buf, m_byte_bound); + } + } +}; + +/* Concrete subclass to complain about buffer overreads. */ + +class buffer_overread : public past_the_end +{ +public: + buffer_overread (const region *reg, tree diag_arg, + byte_range range, tree byte_bound) + : past_the_end (reg, diag_arg, range, byte_bound) + {} + + bool emit (rich_location *rich_loc) final override + { + diagnostic_metadata m; + m.add_cwe (126); + bool warned = warning_meta (rich_loc, m, get_controlling_option (), + "buffer overread"); + + if (warned) + { + char num_bytes_past_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (m_range.m_size_in_bytes, num_bytes_past_buf, UNSIGNED); + if (m_diag_arg) + inform (rich_loc->get_loc (), "write is %s bytes past the end" + " of %qE", num_bytes_past_buf, + m_diag_arg); + else + inform (rich_loc->get_loc (), "write is %s bytes past the end" + "of the region", + num_bytes_past_buf); + } + + return warned; + } + + label_text describe_final_event (const evdesc::final_event &ev) + final override + { + byte_size_t start = m_range.get_start_byte_offset (); + byte_size_t end = m_range.get_last_byte_offset (); + char start_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (start, start_buf, SIGNED); + char end_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (end, end_buf, SIGNED); + + if (start == end) + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds read at byte %s but %qE" + " ends at byte %E", start_buf, m_diag_arg, + m_byte_bound); + return ev.formatted_print ("out-of-bounds read at byte %s but region" + " ends at byte %E", start_buf, + m_byte_bound); + } + else + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds read from byte %s till" + " byte %s but %qE ends at byte %E", + start_buf, end_buf, m_diag_arg, + m_byte_bound); + return ev.formatted_print ("out-of-bounds read from byte %s till" + " byte %s but region ends at byte %E", + start_buf, end_buf, m_byte_bound); + } + } +}; + +/* Concrete subclass to complain about buffer underflows. */ + +class buffer_underflow : public out_of_bounds +{ +public: + buffer_underflow (const region *reg, tree diag_arg, byte_range range) + : out_of_bounds (reg, diag_arg, range) + {} + + bool emit (rich_location *rich_loc) final override + { + diagnostic_metadata m; + m.add_cwe (124); + return warning_meta (rich_loc, m, get_controlling_option (), + "buffer underflow"); + } + + label_text describe_final_event (const evdesc::final_event &ev) + final override + { + byte_size_t start = m_range.get_start_byte_offset (); + byte_size_t end = m_range.get_last_byte_offset (); + char start_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (start, start_buf, SIGNED); + char end_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (end, end_buf, SIGNED); + + if (start == end) + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds write at byte %s but %qE" + " starts at byte 0", start_buf, + m_diag_arg); + return ev.formatted_print ("out-of-bounds write at byte %s but region" + " starts at byte 0", start_buf); + } + else + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds write from byte %s till" + " byte %s but %qE starts at byte 0", + start_buf, end_buf, m_diag_arg); + return ev.formatted_print ("out-of-bounds write from byte %s till" + " byte %s but region starts at byte 0", + start_buf, end_buf);; + } + } +}; + +/* Concrete subclass to complain about buffer underreads. */ + +class buffer_underread : public out_of_bounds +{ +public: + buffer_underread (const region *reg, tree diag_arg, byte_range range) + : out_of_bounds (reg, diag_arg, range) + {} + + bool emit (rich_location *rich_loc) final override + { + diagnostic_metadata m; + m.add_cwe (127); + return warning_meta (rich_loc, m, get_controlling_option (), + "buffer underread"); + } + + label_text describe_final_event (const evdesc::final_event &ev) + final override + { + byte_size_t start = m_range.get_start_byte_offset (); + byte_size_t end = m_range.get_last_byte_offset (); + char start_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (start, start_buf, SIGNED); + char end_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (end, end_buf, SIGNED); + + if (start == end) + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds read at byte %s but %qE" + " starts at byte 0", start_buf, + m_diag_arg); + return ev.formatted_print ("out-of-bounds read at byte %s but region" + " starts at byte 0", start_buf); + } + else + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds read from byte %s till" + " byte %s but %qE starts at byte 0", + start_buf, end_buf, m_diag_arg); + return ev.formatted_print ("out-of-bounds read from byte %s till" + " byte %s but region starts at byte 0", + start_buf, end_buf);; + } + } +}; + +/* May complain when the access on REG is out-of-bounds. */ + +void region_model::check_region_bounds (const region *reg, + enum access_direction dir, + region_model_context *ctxt) const +{ + gcc_assert (ctxt); + + region_offset reg_offset = reg->get_offset (); + const region *base_reg = reg_offset.get_base_region (); + + /* Bail out on symbolic offsets or symbolic regions. + (e.g. because the analyzer did not see previous offsets on the latter, + it might think that a negative access is before the buffer). */ + if (reg_offset.symbolic_p () || base_reg->symbolic_p ()) + return; + byte_offset_t offset = reg_offset.get_bit_offset () >> LOG2_BITS_PER_UNIT; + + /* Find out how many bytes were accessed. */ + const svalue *num_bytes_sval = reg->get_byte_size_sval (m_mgr); + tree num_bytes_tree = num_bytes_sval->maybe_get_constant (); + if (!num_bytes_tree || TREE_CODE (num_bytes_tree) != INTEGER_CST) + /* If we do not know how many bytes were read/written, + assume that at least one byte was read/written. */ + num_bytes_tree = integer_one_node; + + byte_range out (0, 0); + /* NUM_BYTES_TREE should always be interpreted as unsigned. */ + byte_range read_bytes (offset, wi::to_offset (num_bytes_tree).to_uhwi ()); + /* If read_bytes has a subset < 0, we do have an underflow. */ + if (read_bytes.falls_short_of_p (0, &out)) + { + tree diag_arg = get_representative_tree (reg->get_base_region ()); + switch (dir) + { + default: + gcc_unreachable (); + break; + case DIR_READ: + ctxt->warn (new buffer_underread (reg, diag_arg, out)); + break; + case DIR_WRITE: + ctxt->warn (new buffer_underflow (reg, diag_arg, out)); + break; + } + } + + const svalue *capacity = get_capacity (base_reg); + tree cst_capacity_tree = capacity->maybe_get_constant (); + if (!cst_capacity_tree || TREE_CODE (cst_capacity_tree) != INTEGER_CST) + return; + + byte_range buffer (0, wi::to_offset (cst_capacity_tree)); + /* If READ_BYTES exceeds BUFFER, we do have an overflow. */ + if (read_bytes.exceeds_p (buffer, &out)) + { + tree byte_bound = wide_int_to_tree (size_type_node, + buffer.get_next_byte_offset ()); + tree diag_arg = get_representative_tree (reg->get_base_region ()); + + switch (dir) + { + default: + gcc_unreachable (); + break; + case DIR_READ: + ctxt->warn (new buffer_overread (reg, diag_arg, out, byte_bound)); + break; + case DIR_WRITE: + ctxt->warn (new buffer_overflow (reg, diag_arg, out, byte_bound)); + break; + } + } +} + /* Ensure that all arguments at the call described by CD are checked for poisoned values, by calling get_rvalue on each argument. */ @@ -2811,6 +3207,7 @@ region_model::check_region_access (const region *reg, return; check_region_for_taint (reg, dir, ctxt); + check_region_bounds (reg, dir, ctxt); switch (dir) { @@ -3806,6 +4203,19 @@ region_model::get_representative_tree (const svalue *sval) const return fixup_tree_for_diagnostic (expr); } +tree +region_model::get_representative_tree (const region *reg) const +{ + svalue_set visited; + tree expr = get_representative_path_var (reg, &visited).m_tree; + + /* Strip off any top-level cast. */ + if (expr && TREE_CODE (expr) == NOP_EXPR) + expr = TREE_OPERAND (expr, 0); + + return fixup_tree_for_diagnostic (expr); +} + /* Implementation of region_model::get_representative_path_var. Attempt to return a path_var that represents REG, or return diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index 42f8abeb043..1f2c65d46ae 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -732,6 +732,7 @@ class region_model region_model_context *ctxt); tree get_representative_tree (const svalue *sval) const; + tree get_representative_tree (const region *reg) const; path_var get_representative_path_var (const svalue *sval, svalue_set *visited) const; @@ -867,6 +868,8 @@ class region_model region_model_context *ctxt) const; void check_region_size (const region *lhs_reg, const svalue *rhs_sval, region_model_context *ctxt) const; + void check_region_bounds (const region *reg, enum access_direction dir, + region_model_context *ctxt) const; void check_call_args (const call_details &cd) const; void check_external_function_for_access_attr (const gcall *call, diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc index b78bf4ec1b7..fee0ba3bde3 100644 --- a/gcc/analyzer/region.cc +++ b/gcc/analyzer/region.cc @@ -629,6 +629,14 @@ region::symbolic_for_unknown_ptr_p () const return false; } +/* Return true if this is a symbolic region. */ + +bool +region::symbolic_p () const +{ + return get_kind () == RK_SYMBOLIC; +} + /* Return true if this is a region for a decl with name DECL_NAME. Intended for use when debugging (for assertions and conditional breakpoints). */ @@ -1430,6 +1438,30 @@ offset_region::get_relative_concrete_offset (bit_offset_t *out) const return false; } +/* Implementation of region::get_byte_size_sval vfunc for offset_region. */ + +const svalue * +offset_region::get_byte_size_sval (region_model_manager *mgr) const +{ + tree offset_cst = get_byte_offset ()->maybe_get_constant (); + byte_size_t byte_size; + /* If the offset points in the middle of the region, + return the remaining bytes. */ + if (get_byte_size (&byte_size) && offset_cst) + { + byte_size_t offset = wi::to_offset (offset_cst); + byte_range r(0, byte_size); + if (r.contains_p (offset)) + { + tree remaining_byte_size = wide_int_to_tree (size_type_node, + byte_size - offset); + return mgr->get_or_create_constant_svalue (remaining_byte_size); + } + } + + return mgr->get_or_create_unknown_svalue (size_type_node); +} + /* class sized_region : public region. */ /* Implementation of region::accept vfunc for sized_region. */ diff --git a/gcc/analyzer/region.h b/gcc/analyzer/region.h index 1067748a7cd..65efb719cbb 100644 --- a/gcc/analyzer/region.h +++ b/gcc/analyzer/region.h @@ -201,6 +201,8 @@ public: bool symbolic_for_unknown_ptr_p () const; + bool symbolic_p () const; + /* For most base regions it makes sense to track the bindings of the region within the store. As an optimization, some are not tracked (to avoid bloating the store object with redundant binding clusters). */ @@ -907,6 +909,8 @@ public: const svalue *get_byte_offset () const { return m_byte_offset; } bool get_relative_concrete_offset (bit_offset_t *out) const final override; + const svalue * get_byte_size_sval (region_model_manager *mgr) const; + private: const svalue *m_byte_offset; diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc index 46475f6a7fa..848c5e1aaff 100644 --- a/gcc/analyzer/store.cc +++ b/gcc/analyzer/store.cc @@ -424,6 +424,73 @@ byte_range::contains_p (const byte_range &other, byte_range *out) const return false; } +/* Return true if THIS and OTHER intersect and write the number + of bytes both buffers overlap to *OUT_NUM_OVERLAP_BYTES. + + Otherwise return false. */ + +bool +byte_range::intersects_p (const byte_range &other, + byte_size_t *out_num_overlap_bytes) const +{ + if (get_start_byte_offset () < other.get_next_byte_offset () + && other.get_start_byte_offset () < get_next_byte_offset ()) + { + byte_offset_t overlap_start = MAX (get_start_byte_offset (), + other.get_start_byte_offset ()); + byte_offset_t overlap_next = MIN (get_next_byte_offset (), + other.get_next_byte_offset ()); + gcc_assert (overlap_next > overlap_start); + *out_num_overlap_bytes = overlap_next - overlap_start; + return true; + } + else + return false; +} + +/* Return true if THIS exceeds OTHER and write the overhanging + byte range to OUT_OVERHANGING_BYTE_RANGE. */ + +bool +byte_range::exceeds_p (const byte_range &other, + byte_range *out_overhanging_byte_range) const +{ + if (other.get_last_byte_offset () < get_last_byte_offset ()) + { + /* THIS definitely exceeds OTHER. */ + byte_offset_t start = MAX (get_start_byte_offset (), + other.get_next_byte_offset ()); + byte_offset_t size = get_next_byte_offset () - start; + gcc_assert (size > 0); + out_overhanging_byte_range->m_start_byte_offset = start; + out_overhanging_byte_range->m_size_in_bytes = size; + return true; + } + else + return false; +} + +/* Return true if THIS falls short of OFFSET and write the + byte range fallen short to OUT_FALL_SHORT_BYTES. */ + +bool +byte_range::falls_short_of_p (byte_offset_t offset, + byte_range *out_fall_short_bytes) const +{ + if (get_start_byte_offset () < offset) + { + /* THIS falls short of OFFSET. */ + byte_offset_t start = get_start_byte_offset (); + byte_offset_t size = MIN (offset, get_next_byte_offset ()) - start; + gcc_assert (size > 0); + out_fall_short_bytes->m_start_byte_offset = start; + out_fall_short_bytes->m_size_in_bytes = size; + return true; + } + else + return false; +} + /* qsort comparator for byte ranges. */ int diff --git a/gcc/analyzer/store.h b/gcc/analyzer/store.h index 9b54c7b9156..ac8b6853f4b 100644 --- a/gcc/analyzer/store.h +++ b/gcc/analyzer/store.h @@ -310,6 +310,15 @@ struct byte_range && m_size_in_bytes == other.m_size_in_bytes); } + bool intersects_p (const byte_range &other, + byte_size_t *out_num_overlap_bytes) const; + + bool exceeds_p (const byte_range &other, + byte_range *out_overhanging_byte_range) const; + + bool falls_short_of_p (byte_offset_t offset, + byte_range *out_fall_short_bytes) const; + byte_offset_t get_start_byte_offset () const { return m_start_byte_offset; diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index fa23fbeaaaa..5ab834af780 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -459,6 +459,7 @@ Objective-C and Objective-C++ Dialects}. -Wno-analyzer-null-dereference @gol -Wno-analyzer-possible-null-argument @gol -Wno-analyzer-possible-null-dereference @gol +-Wno-analyzer-out-of-bounds @gol -Wno-analyzer-shift-count-negative @gol -Wno-analyzer-shift-count-overflow @gol -Wno-analyzer-stale-setjmp-buffer @gol @@ -9991,6 +9992,17 @@ This warning requires @option{-fanalyzer}, which enables it; use This diagnostic warns for paths through the code in which a value known to be NULL is dereferenced. +@item -Wno-analyzer-out-of-bounds +@opindex Wanalyzer-out-of-bounds +@opindex Wno-analyzer-out-of-bounds +This warning requires @option{-fanalyzer} to enable it; use +@option{-Wno-analyzer-out-of-bounds} to disable it. + +This diagnostic warns for path through the code in which a buffer is +accessed or written out-of-bounds. + +See @url{https://cwe.mitre.org/data/definitions/119.html, CWE-119: Improper Restriction of Operations within the Bounds of a Memory Buffer}. + @item -Wno-analyzer-shift-count-negative @opindex Wanalyzer-shift-count-negative @opindex Wno-analyzer-shift-count-negative diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c index 0c86f09f15b..9590e3137e0 100644 --- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c @@ -1,3 +1,5 @@ +/* { dg-additional-options -Wno-analyzer-out-of-bounds } */ + #include <stdlib.h> #include <stdio.h> #include <stdint.h> diff --git a/gcc/testsuite/gcc.dg/analyzer/memcpy-2.c b/gcc/testsuite/gcc.dg/analyzer/memcpy-2.c index 88ec84cb640..51e4a694951 100644 --- a/gcc/testsuite/gcc.dg/analyzer/memcpy-2.c +++ b/gcc/testsuite/gcc.dg/analyzer/memcpy-2.c @@ -1,4 +1,4 @@ -/* { dg-additional-options "-Wno-stringop-overflow" } */ +/* { dg-additional-options "-Wno-stringop-overflow -Wno-analyzer-out-of-bounds" } */ void main (int c, void *v) diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c new file mode 100644 index 00000000000..715c8b7460f --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c @@ -0,0 +1,119 @@ +#include <stdlib.h> +#include <string.h> +#include <stdint.h> +#include <stdio.h> + +/* Wanalyzer-out-of-bounds tests for buffer overflows. */ + +/* Avoid folding of memcpy. */ +typedef void * (*memcpy_t) (void *dst, const void *src, size_t n); + +static memcpy_t __attribute__((noinline)) +get_memcpy (void) +{ + return memcpy; +} + + +/* Taken from CWE-787. */ +void test1 (void) +{ + int id_sequence[3]; + + id_sequence[0] = 123; + id_sequence[1] = 234; + id_sequence[2] = 345; + id_sequence[3] = 456; /* { dg-line test1 } */ + + /* { dg-warning "overflow" "warning" { target *-*-* } test1 } */ + /* { dg-message "" "note" { target *-*-* } test1 } */ +} + +void test2 (void) +{ + int n = 4; + int arr[4]; + + for (int i = n - 1; i >= 0; i--) + arr[i] = i; +} + +void test3 (void) +{ + int n = 4; + int arr[4]; + + for (int i = n; i >= 0; i--) + arr[i] = i; /* { dg-line test3 } */ + + /* { dg-warning "overflow" "warning" { target *-*-* } test3 } */ + /* { dg-message "" "note" { target *-*-* } test3 } */ +} + +void test4 (void) +{ + int *arr = malloc (4 * sizeof (int)); + if (!arr) + return; + + int *last_el = arr + 3; + *last_el = 3; + + free (arr); +} + +void test5 (void) +{ + int *arr = malloc (4 * sizeof (int)); + if (!arr) + return; + + int *last_el = arr + 4; + *last_el = 4; /* { dg-line test5 } */ + + free (arr); + /* { dg-warning "overflow" "warning" { target *-*-* } test5 } */ + /* { dg-message "" "note" { target *-*-* } test5 } */ +} + +/* Taken from "A Provenance-aware Memory Object Model for C". */ +int y = 2, x = 1; /* { dg-message "capacity" } */ +void test6 (void) +{ + int *p = &x + 1; + int *q = &y; + printf ("Addresses: p=% p q=% p \n" , (void *) p, (void *) q); + if (memcmp (&p , &q , sizeof (p)) == 0) + { + *p = 11; /* { dg-line test6b } */ + printf ("x=%d y=%d *p=%d *q=%d\n" , x, y, *p, *q); /* { dg-line test6c } */ + } + + /* { dg-warning "overflow" "warning" { target *-*-* } test6b } */ + /* { dg-message "" "note" { target *-*-* } test6b } */ + /* { dg-warning "overread" "warning" { target *-*-* } test6c } */ + /* { dg-message "" "note" { target *-*-* } test6c } */ +} + +extern int is_valid (void); + +int returnChunkSize (void *ptr) +{ + /* If chunk info is valid, return the size of usable memory, + else, return -1 to indicate an error. */ + return is_valid () ? sizeof (*ptr) : -1; +} + +/* Taken from CWE-787. */ +void test7 (void) +{ + memcpy_t fn = get_memcpy (); + + int destBuf[4]; + int srcBuf[4]; + fn (destBuf, srcBuf, returnChunkSize (destBuf)); /* { dg-line test7 } */ + + // TODO: Should we handle widening_svalues as a follow-up? + /* { dg-warning "overread.*?overflow|overflow.*?overread" "warning" { xfail *-*-* } test7 } */ + /* { dg-message "" "note" { xfail *-*-* } test7 } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c new file mode 100644 index 00000000000..0df9364c5c1 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c @@ -0,0 +1,83 @@ +#include <stdlib.h> +#include <string.h> +#include <stdio.h> +#include <stdint.h> + +/* Wanalyzer-out-of-bounds tests for buffer overreads. */ + +/* Avoid folding of memcpy. */ +typedef void * (*memcpy_t) (void *dst, const void *src, size_t n); + +static memcpy_t __attribute__((noinline)) +get_memcpy (void) +{ + return memcpy; +} + + +void test1 (void) +{ + int id_sequence[3]; + memset (id_sequence, 0, 3 * sizeof(int)); + printf ("%i", id_sequence[3]); /* { dg-line test1 } */ + + /* { dg-warning "overread" "warning" { target *-*-* } test1 } */ + /* { dg-message "" "note" { target *-*-* } test1 } */ +} + +void test2 (void) +{ + int n = 4; + int arr[n]; + memset (arr, 0, n * sizeof (int)); + + int sum = 0; + for (int i = n - 1; i >= 0; i--) + sum += arr[i]; +} + +void test3 (void) +{ + int n = 4; + int arr[4]; + memset (arr, 0, n * sizeof (int)); + + int sum = 0; + for (int i = n; i > 0; i--) + sum += arr[i]; /* { dg-line test3 } */ + + /* { dg-warning "overread" "warning" { target *-*-* } test3 } */ + /* { dg-message "" "note" { target *-*-* } test3 } */ +} + +void test4 (void) +{ + int n = 4; + int *arr = malloc (n * sizeof (int)); + if (!arr) + return; + memset (arr, 0, n * sizeof(int)); + + int sum = 0; + for (int i = n - 1; i >= 0; i--) + sum += *(arr + i); + + free (arr); +} + +void test5 (void) +{ + int n = 4; + int *arr = malloc (n * sizeof (int)); + if (!arr) + return; + memset (arr, 0, n * sizeof(int)); + + int sum = 0; + for (int i = n; i > 0; i--) + sum += *(arr + i); /* { dg-line test5 } */ + + free (arr); + /* { dg-warning "overread" "warning" { target *-*-* } test5 } */ + /* { dg-message "" "note" { target *-*-* } test5 } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-3.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-3.c new file mode 100644 index 00000000000..90b4a65a067 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-3.c @@ -0,0 +1,91 @@ +#include <stdlib.h> +#include <string.h> +#include <stdint.h> + +/* Wanalyzer-out-of-bounds tests for buffer underreads and writes. */ + +/* Avoid folding of memcpy. */ +typedef void * (*memcpy_t) (void *dst, const void *src, size_t n); +static memcpy_t __attribute__((noinline)) +get_memcpy (void) +{ + return memcpy; +} + + +void test1 (void) +{ + int buf[4]; + int *e = buf - 1; + *e = 42; /* { dg-line test1 } */ + + /* { dg-warning "underflow" "warning" { target *-*-* } test1 } */ + /* { dg-message "" "note" { target *-*-* } test1 } */ +} + +void test2 (void) +{ + int buf[4]; + int *e = buf + 1; + *e = 123; + *(e - 1) = 321; +} + +void test3 (void) +{ + int buf[4]; + int *e = buf + 1; + *e = 123; + *(e - 2) = 321; /* { dg-line test3 } */ + + /* { dg-warning "underflow" "warning" { target *-*-* } test3 } */ + /* { dg-message "" "note" { target *-*-* } test3 } */ +} + +void test4 (void) +{ + memcpy_t fn = get_memcpy (); + int buf[4]; + memset (buf, 1, 4 * sizeof (int)); + int n = -4; + fn (&(buf[n]), buf, sizeof (int)); /* { dg-line test4 } */ + + /* { dg-warning "underflow" "warning" { target *-*-* } test4 } */ + /* { dg-message "" "note" { target *-*-* } test4 } */ +} + +void test5 (void) +{ + int buf[4]; + memset (buf, 1, 4 * sizeof (int)); + + int sum = 0; + for (int i = 4; i >= 0; i++) + sum += *(buf + i); /* { dg-line test5 } */ + + /* { dg-warning "overread" "warning" { target *-*-* } test5 } */ + /* { dg-message "" "note" { target *-*-* } test5 } */ +} + +void test6 (void) +{ + int buf[4]; + memset (buf, 1, 4 * sizeof (int)); + + int *view = buf + 1; + int sum = 0; + for (int i = 0; i < 4; i++) + sum += *(view++); +} + +void test8 (void) +{ + memcpy_t fn = get_memcpy (); + int buf[4]; + memset (buf, 1, 4 * sizeof (int)); + int n = -4; + fn (buf, &(buf[n]), sizeof (int)); /* { dg-line test8 } */ + + /* { dg-warning "underread" "warning" { target *-*-* } test8 } */ + /* { dg-message "" "note" { target *-*-* } test8 } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-container_of.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-container_of.c new file mode 100644 index 00000000000..172ec474c42 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-container_of.c @@ -0,0 +1,51 @@ +/* Further reduced container_of pattern from the Linux Kernel. */ + +struct inner { + /* Don't care */ +}; + +struct outer { + int i; + struct inner inner_struct; +}; + +struct outer *container_of (struct inner *ptr_to_inner) +{ + struct outer *ptr_to_outer = ((struct outer *) (((void *) ptr_to_inner) - __builtin_offsetof(struct outer, inner_struct))); + return ptr_to_outer; +} + +int test (struct outer *outer_p, struct inner *inner_p) +{ + struct outer test; + test.i = 42; + struct inner test2; + int sum = 0; + struct outer *o; + + /* Symbolic inner struct. */ + o = container_of (inner_p); + sum += o->i; // ok + /* Not ok, but we can't be sure that outer + is actually the container of inner. */ + sum += (o - 1)->i; + /* Symbolic outer struct. */ + o = container_of (&(outer_p->inner_struct)); + sum += o->i; // ok + /* Not ok, but indistinguishable from the case above. */ + sum += (o - 1)->i; + /* Concrete outer struct. */ + o = container_of (&(test.inner_struct)); + sum += o->i; // ok + /* Not ok and we do have a concrete region. */ + sum += (o - 1)->i; /* { dg-line testA } */ + /* Concrete inner struct, has no container. */ + o = container_of (&test2); + sum += o->i; /* { dg-line testB } */ + + return sum; + /* { dg-warning "underread" "warning" { target *-*-* } testA } */ + /* { dg-message "" "note" { target *-*-* } testA } */ + /* { dg-warning "underread" "warning" { target *-*-* } testB } */ + /* { dg-message "" "note" { target *-*-* } testB } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-coreutils.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-coreutils.c new file mode 100644 index 00000000000..cd0f4b7f7b2 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-coreutils.c @@ -0,0 +1,29 @@ +/* Reduced from coreutils/ls.c attach. */ + +void add_zero_terminator (char *buf) +{ + char *end = buf; + while (end++); + if (buf < end) + end[-1] = '\0'; +} + +/* Reduced from coreutils/cat.c. */ + +#define LINE_COUNTER_BUF_LEN 20 +static char line_buf[LINE_COUNTER_BUF_LEN] = + { + ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', + ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', '0', + '\t', '\0' + }; + +/* Position of the first digit in 'line_buf'. */ +static char *line_num_start = line_buf + LINE_COUNTER_BUF_LEN - 3; + +static void +next_line_num (void) +{ + if (line_num_start > line_buf) + *--line_num_start = '1'; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-curl.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-curl.c new file mode 100644 index 00000000000..e34b572966e --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-curl.c @@ -0,0 +1,41 @@ +/* { dg-additional-options "-O2" } */ +#include <string.h> + +/* Reduced from curl lib/smb.c. */ +typedef int CURLcode; + +struct smb_conn { + // [...] + char *user; +}; + +struct smb_setup { + // [...] + char bytes[48]; +} __attribute__((packed)); + +struct connectdata { + // [...] + struct smb_conn *smbc; +}; + +CURLcode smb_send_setup (struct connectdata *conn) +{ + struct smb_conn *smbc = conn->smbc; + struct smb_setup msg; + char *p = msg.bytes; + unsigned char lm[24]; + + /* Init to prevent uninit warning. */ + memset(&msg, 0, sizeof(msg)); + memset (&lm, 0, sizeof(lm)); + + memcpy(p, lm, sizeof(lm)); + p += sizeof(lm); + /* Had a false-positive overflow at p. Checker had a number of bytes copied + relative to the start but offset points in the middle the field. */ + strcpy(p, (smbc->user)); + p += strlen(smbc->user) + 1; + + return 1; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr101962.c b/gcc/testsuite/gcc.dg/analyzer/pr101962.c index d15820af27f..1490e7c0d7f 100644 --- a/gcc/testsuite/gcc.dg/analyzer/pr101962.c +++ b/gcc/testsuite/gcc.dg/analyzer/pr101962.c @@ -22,8 +22,9 @@ test_1 (void) a = maybe_inc_int_ptr (a); __analyzer_eval (a == NULL); /* { dg-warning "FALSE" } */ __analyzer_eval (a != NULL); /* { dg-warning "TRUE" } */ - return *a; /* { dg-warning "use of uninitialized value '\\*a'" } */ - /* TODO: a complaint about out-of-bounds would be a better warning. */ + return *a; /* { dg-line test_1 } */ + + /* { dg-warning "overread.*?use of uninitialized value '\\*a'|use of uninitialized value '\\*a'.*?overread" "warning" { target *-*-* } test_1 } */ } static const char * __attribute__((noinline)) diff --git a/gcc/testsuite/gcc.dg/analyzer/pr97029.c b/gcc/testsuite/gcc.dg/analyzer/pr97029.c index ff83ad4d56e..e3b45311779 100644 --- a/gcc/testsuite/gcc.dg/analyzer/pr97029.c +++ b/gcc/testsuite/gcc.dg/analyzer/pr97029.c @@ -1,4 +1,6 @@ -struct vj {}; +struct vj { + char buf[1]; +}; void setjmp (struct vj pl) diff --git a/gcc/testsuite/gcc.dg/analyzer/test-setjmp.h b/gcc/testsuite/gcc.dg/analyzer/test-setjmp.h index db2422709e2..52c57d02b70 100644 --- a/gcc/testsuite/gcc.dg/analyzer/test-setjmp.h +++ b/gcc/testsuite/gcc.dg/analyzer/test-setjmp.h @@ -12,7 +12,9 @@ #pragma GCC system_header -struct __jmp_buf_tag {}; +struct __jmp_buf_tag { + char buf[1]; +}; typedef struct __jmp_buf_tag jmp_buf[1]; typedef struct __jmp_buf_tag sigjmp_buf[1]; diff --git a/gcc/testsuite/gcc.dg/analyzer/zlib-3.c b/gcc/testsuite/gcc.dg/analyzer/zlib-3.c index 5098b4fc363..b05b8629a7f 100644 --- a/gcc/testsuite/gcc.dg/analyzer/zlib-3.c +++ b/gcc/testsuite/gcc.dg/analyzer/zlib-3.c @@ -182,7 +182,9 @@ static int huft_build(uInt *b, uInt n, uInt s, const uInt *d, const uInt *e, q[j] = r; /* { dg-warning "use of uninitialized value 'r.base'" } */ mask = (1 << w) - 1; - while ((i & mask) != x[h]) { + /* The analyzer thinks that h can be -1 here. + This is probably a false positive. */ + while ((i & mask) != x[h]) { /* { dg-bogus "underread" "" { xfail *-*-* } } */ h--; w -= l; mask = (1 << w) - 1; -- 2.37.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] analyzer: out-of-bounds checker [PR106000] 2022-08-09 21:19 ` [PATCH 2/2] analyzer: out-of-bounds checker [PR106000] Tim Lange @ 2022-08-09 22:44 ` David Malcolm 0 siblings, 0 replies; 8+ messages in thread From: David Malcolm @ 2022-08-09 22:44 UTC (permalink / raw) To: Tim Lange, gcc-patches On Tue, 2022-08-09 at 23:19 +0200, Tim Lange wrote: > This patch adds an experimental out-of-bounds checker to the > analyzer. > > The checker was tested on coreutils, curl, httpd and openssh. It is > mostly > accurate but does produce false-positives on yacc-generated files and > sometimes when the analyzer misses an invariant. These cases will be > documented in bugzilla. > (Regrtests still running with the latest changes, will report back > later.) Hi Tim, thanks for the patch, and for all the testing you've done on it. We've already had several rounds of review of this off-list, and this patch looks very close to ready. Some nits below... > diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt > index 5021376b6fb..8e73af60ceb 100644 > --- a/gcc/analyzer/analyzer.opt > +++ b/gcc/analyzer/analyzer.opt > @@ -158,6 +158,10 @@ Wanalyzer-tainted-size > Common Var(warn_analyzer_tainted_size) Init(1) Warning > Warn about code paths in which an unsanitized value is used as a > size. > > +Wanalyzer-out-of-bounds > +Common Var(warn_analyzer_out_of_bounds) Init(1) Warning > +Warn about code paths in which a write or read to a buffer is out- > of-bounds. > + Please keep the list alphabetized; I think this needs to be between Wanalyzer-mismatching-deallocation and Wanalyzer-possible-null-argument > Wanalyzer-use-after-free > Common Var(warn_analyzer_use_after_free) Init(1) Warning > Warn about code paths in which a freed value is used. > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region- > model.cc > index f7df2fca245..2f9382ed96c 100644 > --- a/gcc/analyzer/region-model.cc > +++ b/gcc/analyzer/region-model.cc > @@ -1268,6 +1268,402 @@ region_model::on_stmt_pre (const gimple > *stmt, > } > } > > +/* Abstract base class for all out-of-bounds warnings. */ > + > +class out_of_bounds : public > pending_diagnostic_subclass<out_of_bounds> > +{ > +public: > + out_of_bounds (const region *reg, tree diag_arg, byte_range range) > + : m_reg (reg), m_diag_arg (diag_arg), m_range (range) > + {} > + > + const char *get_kind () const final override > + { > + return "out_of_bounds_diagnostic"; > + } > + > + bool operator== (const out_of_bounds &other) const > + { > + return m_reg == other.m_reg > + && m_range == other.m_range > + && pending_diagnostic::same_tree_p (m_diag_arg, > other.m_diag_arg); > + } > + > + int get_controlling_option () const final override > + { > + return OPT_Wanalyzer_out_of_bounds; > + } > + > + void mark_interesting_stuff (interesting_t *interest) final > override > + { > + interest->add_region_creation (m_reg); > + } > + > +protected: > + const region *m_reg; > + tree m_diag_arg; > + byte_range m_range; Please add a comment clarifying what the meaning of m_range is here. Is it (a) the range of all bytes that are accessed, (b) the range of bytes that are accessed out-of-bounds, (c) etc? From my reading of the patch I think it's (b). > +}; > + > +/* Abstract subclass to complaing about out-of-bounds > + past the end of the buffer. */ > + > +class past_the_end : public out_of_bounds > +{ > +public: > + past_the_end (const region *reg, tree diag_arg, byte_range range, > + tree byte_bound) > + : out_of_bounds (reg, diag_arg, range), m_byte_bound (byte_bound) > + {} > + > + bool operator== (const past_the_end &other) const > + { > + return m_reg == other.m_reg > + && m_range == other.m_range > + && pending_diagnostic::same_tree_p (m_diag_arg, > other.m_diag_arg) Is it possible to call out_of_bounds::operator== for the first three fields, rather than a copy-and-paste of the logic? > + && pending_diagnostic::same_tree_p (m_byte_bound, > + other.m_byte_bound); > + } > + > + label_text > + describe_region_creation_event (const evdesc::region_creation &ev) > final > + override > + { > + if (m_byte_bound && TREE_CODE (m_byte_bound) == INTEGER_CST) > + return ev.formatted_print ("capacity is %E bytes", > m_byte_bound); > + > + return label_text (); > + } > + > +protected: > + tree m_byte_bound; > +}; [...snip the concrete subclasses...] We went through several rounds of review off-list, and I have lots of ideas for wording tweaks to the patch, but rather than me be a "backseat driver" (or bikeshedding), I think that that aspect of the patch is good enough as-is, and I'll make the wording changes myself once the patch is in trunk. [...snip...] > + > + if (warned) > + { > + char num_bytes_past_buf[WIDE_INT_PRINT_BUFFER_SIZE]; > + print_dec (m_range.m_size_in_bytes, num_bytes_past_buf, > UNSIGNED); I think we can use %wu for this, but I can fix this up in a followup. [...snip...] > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index fa23fbeaaaa..5ab834af780 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -459,6 +459,7 @@ Objective-C and Objective-C++ Dialects}. > -Wno-analyzer-null-dereference @gol > -Wno-analyzer-possible-null-argument @gol > -Wno-analyzer-possible-null-dereference @gol > +-Wno-analyzer-out-of-bounds @gol Please move between -Wno-analyzer-null-dereference @gol and -Wno-analyzer-possible-null-argument @gol for alphabetization. > -Wno-analyzer-shift-count-negative @gol > -Wno-analyzer-shift-count-overflow @gol > -Wno-analyzer-stale-setjmp-buffer @gol > @@ -9991,6 +9992,17 @@ This warning requires @option{-fanalyzer}, > which enables it; use > This diagnostic warns for paths through the code in which a > value known to be NULL is dereferenced. > > +@item -Wno-analyzer-out-of-bounds > +@opindex Wanalyzer-out-of-bounds > +@opindex Wno-analyzer-out-of-bounds > +This warning requires @option{-fanalyzer} to enable it; use > +@option{-Wno-analyzer-out-of-bounds} to disable it. > + > +This diagnostic warns for path through the code in which a buffer is > +accessed or written out-of-bounds. Would be good to clarify the limitations: as I understand it: "The diagnostic only applies for cases where the analyzer is able to determine a constant size for the buffer. It warns when any part of a read or write is definitely before the start of the buffer, or definitely after the end." ...or somesuch wording. > + > +See @url{https://cwe.mitre.org/data/definitions/119.html, CWE-119: > Improper Restriction of Operations within the Bounds of a Memory > Buffer}. Also, please move the new entry to position to keep things alphabetized. > + > @item -Wno-analyzer-shift-count-negative > @opindex Wanalyzer-shift-count-negative > @opindex Wno-analyzer-shift-count-negative [...snip...] > diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c > b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c > new file mode 100644 > index 00000000000..715c8b7460f > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c > @@ -0,0 +1,119 @@ > +#include <stdlib.h> > +#include <string.h> > +#include <stdint.h> > +#include <stdio.h> > + > +/* Wanalyzer-out-of-bounds tests for buffer overflows. */ > + > +/* Avoid folding of memcpy. */ > +typedef void * (*memcpy_t) (void *dst, const void *src, size_t n); > + > +static memcpy_t __attribute__((noinline)) > +get_memcpy (void) > +{ > + return memcpy; > +} > + > + > +/* Taken from CWE-787. */ > +void test1 (void) > +{ > + int id_sequence[3]; > + > + id_sequence[0] = 123; > + id_sequence[1] = 234; > + id_sequence[2] = 345; > + id_sequence[3] = 456; /* { dg-line test1 } */ > + > + /* { dg-warning "overflow" "warning" { target *-*-* } test1 } */ > + /* { dg-message "" "note" { target *-*-* } test1 } */ I see that you've left the regexes mostly blank in the various DejaGnu directives in these new tests. Normally I'd want these to be less vague, but given that I plan to change the wordings in a followup anyway, this is OK. [...snip lots of great testcases...] With the above nits fixed, the patch is OK for trunk (assuming that your testing doesn't show any problems). Thanks again for the patch; this feels like a major new feature. Dave ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] analyzer: consider that realloc could shrink the buffer [PR106539] 2022-08-09 21:19 [PATCH 1/2] analyzer: consider that realloc could shrink the buffer [PR106539] Tim Lange 2022-08-09 21:19 ` [PATCH 2/2] analyzer: out-of-bounds checker [PR106000] Tim Lange @ 2022-08-09 22:02 ` David Malcolm 2022-08-11 17:24 ` [PATCH 1/2 v2] " Tim Lange 2 siblings, 0 replies; 8+ messages in thread From: David Malcolm @ 2022-08-09 22:02 UTC (permalink / raw) To: Tim Lange, gcc-patches On Tue, 2022-08-09 at 23:19 +0200, Tim Lange wrote: > This patch adds the "shrinks buffer" case to the success_with_move > modelling of realloc. Hi Tim, thanks for the patch. > > 2022-08-09 Tim Lange <mail@tim-lange.me> > > gcc/analyzer/ChangeLog: > > PR analyzer/106539 > * region-model-impl-calls.cc > (region_model::impl_call_realloc): > Add get_copied_size function and pass the result as the size > of the > new sized_region. > > --- > gcc/analyzer/region-model-impl-calls.cc | 37 > ++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) > > diff --git a/gcc/analyzer/region-model-impl-calls.cc > b/gcc/analyzer/region-model-impl-calls.cc > index 8c38e9206fa..50a19a52a21 100644 > --- a/gcc/analyzer/region-model-impl-calls.cc > +++ b/gcc/analyzer/region-model-impl-calls.cc > @@ -737,9 +737,11 @@ 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 > ()); > + const svalue *copied_size_sval > + = get_copied_size (old_size_sval, new_size_sval); > const region *sized_new_reg > = model->m_mgr->get_sized_region (new_reg, NULL, > - old_size_sval); > + copied_size_sval); I think that we need to use the same copied size svalue for both sized_old_reg and sized_new_reg, so that we're using a consistent size when getting buffer_content_sval. (I admit my handling of symbolic sizes of svalues is a bit sloppy, but I'd prefer not to add extra inconsistencies here) So the copied_size_sval determination needs to happen before getting sized_old_reg. Also, I think renaming sized_{old,new}_reg to copied_{old,new}_reg might make things clearer (we're copying from a leading subset of the old region to a leading subset of the new region). > model->set_value (sized_new_reg, buffer_content_sval, > cd.get_ctxt ()); > } > @@ -774,6 +776,39 @@ region_model::impl_call_realloc (const > call_details &cd) > else > return true; > } > + > + private: > + /* Return the size svalue for the new region allocated by > realloc. */ This comment is misleading - isn't it the size of the existing data to be copied, i.e. the lesser of the old and new sizes? (allowing for the fact that one or both could be symbolic, of course) Please also add a simple explicit test case for the shrinking case (IIRC from our offlist discussion there's already one that happened to cover shrinking, but I think that testcase was doing other things too). Does the patch fix the missing leak warning for the test case in PR106539? If so, please have the patch add that to the test suite. Thanks again Dave > + 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. */ ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2 v2] analyzer: consider that realloc could shrink the buffer [PR106539] 2022-08-09 21:19 [PATCH 1/2] analyzer: consider that realloc could shrink the buffer [PR106539] Tim Lange 2022-08-09 21:19 ` [PATCH 2/2] analyzer: out-of-bounds checker [PR106000] Tim Lange 2022-08-09 22:02 ` [PATCH 1/2] analyzer: consider that realloc could shrink the buffer [PR106539] David Malcolm @ 2022-08-11 17:24 ` Tim Lange 2022-08-11 17:24 ` [PATCH 2/2 v2] analyzer: out-of-bounds checker [PR106000] Tim Lange 2022-08-11 19:25 ` [PATCH 1/2 v2] analyzer: consider that realloc could shrink the buffer [PR106539] David Malcolm 2 siblings, 2 replies; 8+ messages in thread From: Tim Lange @ 2022-08-11 17:24 UTC (permalink / raw) To: gcc-patches; +Cc: dmalcolm, Tim Lange 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 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); +} -- 2.37.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2 v2] analyzer: out-of-bounds checker [PR106000] 2022-08-11 17:24 ` [PATCH 1/2 v2] " Tim Lange @ 2022-08-11 17:24 ` Tim Lange 2022-08-11 19:30 ` David Malcolm 2022-08-11 19:25 ` [PATCH 1/2 v2] analyzer: consider that realloc could shrink the buffer [PR106539] David Malcolm 1 sibling, 1 reply; 8+ messages in thread From: Tim Lange @ 2022-08-11 17:24 UTC (permalink / raw) To: gcc-patches; +Cc: dmalcolm, Tim Lange This patch adds an experimental out-of-bounds checker to the analyzer. The checker was tested on coreutils, curl, httpd and openssh. It is mostly accurate but does produce false-positives on yacc-generated files and sometimes when the analyzer misses an invariant. These cases will be documented in bugzilla. 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/106000 * analyzer.opt: Add Wanalyzer-out-of-bounds. * region-model.cc (class out_of_bounds): Diagnostics base class for all out-of-bounds diagnostics. (class past_the_end): Base class derived from out_of_bounds for the buffer_overflow and buffer_overread diagnostics. (class buffer_overflow): Buffer overflow diagnostics. (class buffer_overread): Buffer overread diagnostics. (class buffer_underflow): Buffer underflow diagnostics. (class buffer_underread): Buffer overread diagnostics. (region_model::check_region_bounds): New function to check region bounds for out-of-bounds accesses. (region_model::check_region_access): Add call to check_region_bounds. (region_model::get_representative_tree): New function that accepts a region instead of an svalue. * region-model.h (class region_model): Add region_model::check_region_bounds. * region.cc (region::symbolic_p): New predicate. (offset_region::get_byte_size_sval): Only return the remaining byte size on offset_regions. * region.h: Add region::symbolic_p. * store.cc (byte_range::intersects_p): Add new function equivalent to bit_range::intersects_p. (byte_range::exceeds_p): New function. (byte_range::falls_short_of_p): New function. * store.h (struct byte_range): Add byte_range::intersects_p, byte_range::exceeds_p and byte_range::falls_short_of_p. gcc/ChangeLog: PR analyzer/106000 * doc/invoke.texi: Add Wanalyzer-out-of-bounds. gcc/testsuite/ChangeLog: PR analyzer/106000 * g++.dg/analyzer/pr100244.C: Disable out-of-bounds warning. * gcc.dg/analyzer/allocation-size-3.c: Disable out-of-bounds warning. * gcc.dg/analyzer/memcpy-2.c: Disable out-of-bounds warning. * gcc.dg/analyzer/pr101962.c: Add dg-warning. * gcc.dg/analyzer/pr96764.c: Disable out-of-bounds warning. * gcc.dg/analyzer/pr97029.c: Add dummy buffer to prevent an out-of-bounds warning. * gcc.dg/analyzer/test-setjmp.h: Add dummy buffer to prevent an out-of-bounds warning. * gcc.dg/analyzer/zlib-3.c: Add dg-bogus. * g++.dg/analyzer/out-of-bounds-placement-new.C: New test. * gcc.dg/analyzer/out-of-bounds-1.c: New test. * gcc.dg/analyzer/out-of-bounds-2.c: New test. * gcc.dg/analyzer/out-of-bounds-3.c: New test. * gcc.dg/analyzer/out-of-bounds-container_of.c: New test. * gcc.dg/analyzer/out-of-bounds-coreutils.c: New test. * gcc.dg/analyzer/out-of-bounds-curl.c: New test. --- gcc/analyzer/analyzer.opt | 4 + gcc/analyzer/region-model.cc | 422 ++++++++++++++++++ gcc/analyzer/region-model.h | 3 + gcc/analyzer/region.cc | 32 ++ gcc/analyzer/region.h | 4 + gcc/analyzer/store.cc | 67 +++ gcc/analyzer/store.h | 9 + gcc/doc/invoke.texi | 15 + .../analyzer/out-of-bounds-placement-new.C | 19 + gcc/testsuite/g++.dg/analyzer/pr100244.C | 5 +- .../gcc.dg/analyzer/allocation-size-3.c | 2 + gcc/testsuite/gcc.dg/analyzer/memcpy-2.c | 2 +- .../gcc.dg/analyzer/out-of-bounds-1.c | 120 +++++ .../gcc.dg/analyzer/out-of-bounds-2.c | 83 ++++ .../gcc.dg/analyzer/out-of-bounds-3.c | 91 ++++ .../analyzer/out-of-bounds-container_of.c | 51 +++ .../gcc.dg/analyzer/out-of-bounds-coreutils.c | 29 ++ .../gcc.dg/analyzer/out-of-bounds-curl.c | 41 ++ gcc/testsuite/gcc.dg/analyzer/pr101962.c | 6 +- gcc/testsuite/gcc.dg/analyzer/pr96764.c | 2 + gcc/testsuite/gcc.dg/analyzer/pr97029.c | 4 +- gcc/testsuite/gcc.dg/analyzer/test-setjmp.h | 4 +- gcc/testsuite/gcc.dg/analyzer/zlib-3.c | 4 +- 23 files changed, 1012 insertions(+), 7 deletions(-) create mode 100644 gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-3.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-container_of.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-coreutils.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-curl.c diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt index 5021376b6fb..b2a631bb9db 100644 --- a/gcc/analyzer/analyzer.opt +++ b/gcc/analyzer/analyzer.opt @@ -106,6 +106,10 @@ Wanalyzer-mismatching-deallocation Common Var(warn_analyzer_mismatching_deallocation) Init(1) Warning Warn about code paths in which the wrong deallocation function is called. +Wanalyzer-out-of-bounds +Common Var(warn_analyzer_out_of_bounds) Init(1) Warning +Warn about code paths in which a write or read to a buffer is out-of-bounds. + Wanalyzer-possible-null-argument Common Var(warn_analyzer_possible_null_argument) Init(1) Warning Warn about code paths in which a possibly-NULL value is passed to a must-not-be-NULL function argument. diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index f7df2fca245..a5ed19be2ff 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -1268,6 +1268,414 @@ region_model::on_stmt_pre (const gimple *stmt, } } +/* Abstract base class for all out-of-bounds warnings. */ + +class out_of_bounds : public pending_diagnostic_subclass<out_of_bounds> +{ +public: + out_of_bounds (const region *reg, tree diag_arg, + byte_range out_of_bounds_range) + : m_reg (reg), m_diag_arg (diag_arg), + m_out_of_bounds_range (out_of_bounds_range) + {} + + const char *get_kind () const final override + { + return "out_of_bounds_diagnostic"; + } + + bool operator== (const out_of_bounds &other) const + { + return m_reg == other.m_reg + && m_out_of_bounds_range == other.m_out_of_bounds_range + && pending_diagnostic::same_tree_p (m_diag_arg, other.m_diag_arg); + } + + int get_controlling_option () const final override + { + return OPT_Wanalyzer_out_of_bounds; + } + + void mark_interesting_stuff (interesting_t *interest) final override + { + interest->add_region_creation (m_reg); + } + +protected: + const region *m_reg; + tree m_diag_arg; + byte_range m_out_of_bounds_range; +}; + +/* Abstract subclass to complaing about out-of-bounds + past the end of the buffer. */ + +class past_the_end : public out_of_bounds +{ +public: + past_the_end (const region *reg, tree diag_arg, byte_range range, + tree byte_bound) + : out_of_bounds (reg, diag_arg, range), m_byte_bound (byte_bound) + {} + + bool operator== (const past_the_end &other) const + { + return out_of_bounds::operator== (other) + && pending_diagnostic::same_tree_p (m_byte_bound, + other.m_byte_bound); + } + + label_text + describe_region_creation_event (const evdesc::region_creation &ev) final + override + { + if (m_byte_bound && TREE_CODE (m_byte_bound) == INTEGER_CST) + return ev.formatted_print ("capacity is %E bytes", m_byte_bound); + + return label_text (); + } + +protected: + tree m_byte_bound; +}; + +/* Concrete subclass to complain about buffer overflows. */ + +class buffer_overflow : public past_the_end +{ +public: + buffer_overflow (const region *reg, tree diag_arg, + byte_range range, tree byte_bound) + : past_the_end (reg, diag_arg, range, byte_bound) + {} + + bool emit (rich_location *rich_loc) final override + { + diagnostic_metadata m; + bool warned; + switch (m_reg->get_memory_space ()) + { + default: + m.add_cwe (787); + warned = warning_meta (rich_loc, m, get_controlling_option (), + "buffer overflow"); + break; + case MEMSPACE_STACK: + m.add_cwe (121); + warned = warning_meta (rich_loc, m, get_controlling_option (), + "stack-based buffer overflow"); + break; + case MEMSPACE_HEAP: + m.add_cwe (122); + warned = warning_meta (rich_loc, m, get_controlling_option (), + "heap-based buffer overflow"); + break; + } + + if (warned) + { + char num_bytes_past_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (m_out_of_bounds_range.m_size_in_bytes, + num_bytes_past_buf, UNSIGNED); + if (m_diag_arg) + inform (rich_loc->get_loc (), "write is %s bytes past the end" + " of %qE", num_bytes_past_buf, + m_diag_arg); + else + inform (rich_loc->get_loc (), "write is %s bytes past the end" + "of the region", + num_bytes_past_buf); + } + + return warned; + } + + label_text describe_final_event (const evdesc::final_event &ev) + final override + { + byte_size_t start = m_out_of_bounds_range.get_start_byte_offset (); + byte_size_t end = m_out_of_bounds_range.get_last_byte_offset (); + char start_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (start, start_buf, SIGNED); + char end_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (end, end_buf, SIGNED); + + if (start == end) + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds write at byte %s but %qE" + " ends at byte %E", start_buf, m_diag_arg, + m_byte_bound); + return ev.formatted_print ("out-of-bounds write at byte %s but region" + " ends at byte %E", start_buf, + m_byte_bound); + } + else + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds write from byte %s till" + " byte %s but %qE ends at byte %E", + start_buf, end_buf, m_diag_arg, + m_byte_bound); + return ev.formatted_print ("out-of-bounds write from byte %s till" + " byte %s but region ends at byte %E", + start_buf, end_buf, m_byte_bound); + } + } +}; + +/* Concrete subclass to complain about buffer overreads. */ + +class buffer_overread : public past_the_end +{ +public: + buffer_overread (const region *reg, tree diag_arg, + byte_range range, tree byte_bound) + : past_the_end (reg, diag_arg, range, byte_bound) + {} + + bool emit (rich_location *rich_loc) final override + { + diagnostic_metadata m; + m.add_cwe (126); + bool warned = warning_meta (rich_loc, m, get_controlling_option (), + "buffer overread"); + + if (warned) + { + char num_bytes_past_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (m_out_of_bounds_range.m_size_in_bytes, + num_bytes_past_buf, UNSIGNED); + if (m_diag_arg) + inform (rich_loc->get_loc (), "write is %s bytes past the end" + " of %qE", num_bytes_past_buf, + m_diag_arg); + else + inform (rich_loc->get_loc (), "write is %s bytes past the end" + "of the region", + num_bytes_past_buf); + } + + return warned; + } + + label_text describe_final_event (const evdesc::final_event &ev) + final override + { + byte_size_t start = m_out_of_bounds_range.get_start_byte_offset (); + byte_size_t end = m_out_of_bounds_range.get_last_byte_offset (); + char start_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (start, start_buf, SIGNED); + char end_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (end, end_buf, SIGNED); + + if (start == end) + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds read at byte %s but %qE" + " ends at byte %E", start_buf, m_diag_arg, + m_byte_bound); + return ev.formatted_print ("out-of-bounds read at byte %s but region" + " ends at byte %E", start_buf, + m_byte_bound); + } + else + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds read from byte %s till" + " byte %s but %qE ends at byte %E", + start_buf, end_buf, m_diag_arg, + m_byte_bound); + return ev.formatted_print ("out-of-bounds read from byte %s till" + " byte %s but region ends at byte %E", + start_buf, end_buf, m_byte_bound); + } + } +}; + +/* Concrete subclass to complain about buffer underflows. */ + +class buffer_underflow : public out_of_bounds +{ +public: + buffer_underflow (const region *reg, tree diag_arg, byte_range range) + : out_of_bounds (reg, diag_arg, range) + {} + + bool emit (rich_location *rich_loc) final override + { + diagnostic_metadata m; + m.add_cwe (124); + return warning_meta (rich_loc, m, get_controlling_option (), + "buffer underflow"); + } + + label_text describe_final_event (const evdesc::final_event &ev) + final override + { + byte_size_t start = m_out_of_bounds_range.get_start_byte_offset (); + byte_size_t end = m_out_of_bounds_range.get_last_byte_offset (); + char start_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (start, start_buf, SIGNED); + char end_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (end, end_buf, SIGNED); + + if (start == end) + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds write at byte %s but %qE" + " starts at byte 0", start_buf, + m_diag_arg); + return ev.formatted_print ("out-of-bounds write at byte %s but region" + " starts at byte 0", start_buf); + } + else + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds write from byte %s till" + " byte %s but %qE starts at byte 0", + start_buf, end_buf, m_diag_arg); + return ev.formatted_print ("out-of-bounds write from byte %s till" + " byte %s but region starts at byte 0", + start_buf, end_buf);; + } + } +}; + +/* Concrete subclass to complain about buffer underreads. */ + +class buffer_underread : public out_of_bounds +{ +public: + buffer_underread (const region *reg, tree diag_arg, byte_range range) + : out_of_bounds (reg, diag_arg, range) + {} + + bool emit (rich_location *rich_loc) final override + { + diagnostic_metadata m; + m.add_cwe (127); + return warning_meta (rich_loc, m, get_controlling_option (), + "buffer underread"); + } + + label_text describe_final_event (const evdesc::final_event &ev) + final override + { + byte_size_t start = m_out_of_bounds_range.get_start_byte_offset (); + byte_size_t end = m_out_of_bounds_range.get_last_byte_offset (); + char start_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (start, start_buf, SIGNED); + char end_buf[WIDE_INT_PRINT_BUFFER_SIZE]; + print_dec (end, end_buf, SIGNED); + + if (start == end) + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds read at byte %s but %qE" + " starts at byte 0", start_buf, + m_diag_arg); + return ev.formatted_print ("out-of-bounds read at byte %s but region" + " starts at byte 0", start_buf); + } + else + { + if (m_diag_arg) + return ev.formatted_print ("out-of-bounds read from byte %s till" + " byte %s but %qE starts at byte 0", + start_buf, end_buf, m_diag_arg); + return ev.formatted_print ("out-of-bounds read from byte %s till" + " byte %s but region starts at byte 0", + start_buf, end_buf);; + } + } +}; + +/* May complain when the access on REG is out-of-bounds. */ + +void region_model::check_region_bounds (const region *reg, + enum access_direction dir, + region_model_context *ctxt) const +{ + gcc_assert (ctxt); + + region_offset reg_offset = reg->get_offset (); + const region *base_reg = reg_offset.get_base_region (); + + /* Bail out on symbolic offsets or symbolic regions. + (e.g. because the analyzer did not see previous offsets on the latter, + it might think that a negative access is before the buffer). */ + if (reg_offset.symbolic_p () || base_reg->symbolic_p ()) + return; + byte_offset_t offset_unsigned + = reg_offset.get_bit_offset () >> LOG2_BITS_PER_UNIT; + /* The constant offset from a pointer is represented internally as a sizetype + but should be interpreted as a signed value here. The statement below + converts the offset to a signed integer with the same precision the + sizetype has on the target system. + + For example, this is needed for out-of-bounds-3.c test1 to pass when + compiled with a 64-bit gcc build targeting 32-bit systems. */ + byte_offset_t offset + = offset_unsigned.to_shwi (TYPE_PRECISION (size_type_node)); + + /* Find out how many bytes were accessed. */ + const svalue *num_bytes_sval = reg->get_byte_size_sval (m_mgr); + tree num_bytes_tree = num_bytes_sval->maybe_get_constant (); + if (!num_bytes_tree || TREE_CODE (num_bytes_tree) != INTEGER_CST) + /* If we do not know how many bytes were read/written, + assume that at least one byte was read/written. */ + num_bytes_tree = integer_one_node; + + byte_range out (0, 0); + /* NUM_BYTES_TREE should always be interpreted as unsigned. */ + byte_range read_bytes (offset, wi::to_offset (num_bytes_tree).to_uhwi ()); + /* If read_bytes has a subset < 0, we do have an underflow. */ + if (read_bytes.falls_short_of_p (0, &out)) + { + tree diag_arg = get_representative_tree (reg->get_base_region ()); + switch (dir) + { + default: + gcc_unreachable (); + break; + case DIR_READ: + ctxt->warn (new buffer_underread (reg, diag_arg, out)); + break; + case DIR_WRITE: + ctxt->warn (new buffer_underflow (reg, diag_arg, out)); + break; + } + } + + const svalue *capacity = get_capacity (base_reg); + tree cst_capacity_tree = capacity->maybe_get_constant (); + if (!cst_capacity_tree || TREE_CODE (cst_capacity_tree) != INTEGER_CST) + return; + + byte_range buffer (0, wi::to_offset (cst_capacity_tree)); + /* If READ_BYTES exceeds BUFFER, we do have an overflow. */ + if (read_bytes.exceeds_p (buffer, &out)) + { + tree byte_bound = wide_int_to_tree (size_type_node, + buffer.get_next_byte_offset ()); + tree diag_arg = get_representative_tree (reg->get_base_region ()); + + switch (dir) + { + default: + gcc_unreachable (); + break; + case DIR_READ: + ctxt->warn (new buffer_overread (reg, diag_arg, out, byte_bound)); + break; + case DIR_WRITE: + ctxt->warn (new buffer_overflow (reg, diag_arg, out, byte_bound)); + break; + } + } +} + /* Ensure that all arguments at the call described by CD are checked for poisoned values, by calling get_rvalue on each argument. */ @@ -2811,6 +3219,7 @@ region_model::check_region_access (const region *reg, return; check_region_for_taint (reg, dir, ctxt); + check_region_bounds (reg, dir, ctxt); switch (dir) { @@ -3806,6 +4215,19 @@ region_model::get_representative_tree (const svalue *sval) const return fixup_tree_for_diagnostic (expr); } +tree +region_model::get_representative_tree (const region *reg) const +{ + svalue_set visited; + tree expr = get_representative_path_var (reg, &visited).m_tree; + + /* Strip off any top-level cast. */ + if (expr && TREE_CODE (expr) == NOP_EXPR) + expr = TREE_OPERAND (expr, 0); + + return fixup_tree_for_diagnostic (expr); +} + /* Implementation of region_model::get_representative_path_var. Attempt to return a path_var that represents REG, or return diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index 42f8abeb043..1f2c65d46ae 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -732,6 +732,7 @@ class region_model region_model_context *ctxt); tree get_representative_tree (const svalue *sval) const; + tree get_representative_tree (const region *reg) const; path_var get_representative_path_var (const svalue *sval, svalue_set *visited) const; @@ -867,6 +868,8 @@ class region_model region_model_context *ctxt) const; void check_region_size (const region *lhs_reg, const svalue *rhs_sval, region_model_context *ctxt) const; + void check_region_bounds (const region *reg, enum access_direction dir, + region_model_context *ctxt) const; void check_call_args (const call_details &cd) const; void check_external_function_for_access_attr (const gcall *call, diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc index b78bf4ec1b7..04c00266b46 100644 --- a/gcc/analyzer/region.cc +++ b/gcc/analyzer/region.cc @@ -629,6 +629,14 @@ region::symbolic_for_unknown_ptr_p () const return false; } +/* Return true if this is a symbolic region. */ + +bool +region::symbolic_p () const +{ + return get_kind () == RK_SYMBOLIC; +} + /* Return true if this is a region for a decl with name DECL_NAME. Intended for use when debugging (for assertions and conditional breakpoints). */ @@ -1430,6 +1438,30 @@ offset_region::get_relative_concrete_offset (bit_offset_t *out) const return false; } +/* Implementation of region::get_byte_size_sval vfunc for offset_region. */ + +const svalue * +offset_region::get_byte_size_sval (region_model_manager *mgr) const +{ + tree offset_cst = get_byte_offset ()->maybe_get_constant (); + byte_size_t byte_size; + /* If the offset points in the middle of the region, + return the remaining bytes. */ + if (get_byte_size (&byte_size) && offset_cst) + { + byte_size_t offset = wi::to_offset (offset_cst); + byte_range r(0, byte_size); + if (r.contains_p (offset)) + { + tree remaining_byte_size = wide_int_to_tree (size_type_node, + byte_size - offset); + return mgr->get_or_create_constant_svalue (remaining_byte_size); + } + } + + return region::get_byte_size_sval (mgr); +} + /* class sized_region : public region. */ /* Implementation of region::accept vfunc for sized_region. */ diff --git a/gcc/analyzer/region.h b/gcc/analyzer/region.h index 1067748a7cd..65efb719cbb 100644 --- a/gcc/analyzer/region.h +++ b/gcc/analyzer/region.h @@ -201,6 +201,8 @@ public: bool symbolic_for_unknown_ptr_p () const; + bool symbolic_p () const; + /* For most base regions it makes sense to track the bindings of the region within the store. As an optimization, some are not tracked (to avoid bloating the store object with redundant binding clusters). */ @@ -907,6 +909,8 @@ public: const svalue *get_byte_offset () const { return m_byte_offset; } bool get_relative_concrete_offset (bit_offset_t *out) const final override; + const svalue * get_byte_size_sval (region_model_manager *mgr) const; + private: const svalue *m_byte_offset; diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc index 46475f6a7fa..848c5e1aaff 100644 --- a/gcc/analyzer/store.cc +++ b/gcc/analyzer/store.cc @@ -424,6 +424,73 @@ byte_range::contains_p (const byte_range &other, byte_range *out) const return false; } +/* Return true if THIS and OTHER intersect and write the number + of bytes both buffers overlap to *OUT_NUM_OVERLAP_BYTES. + + Otherwise return false. */ + +bool +byte_range::intersects_p (const byte_range &other, + byte_size_t *out_num_overlap_bytes) const +{ + if (get_start_byte_offset () < other.get_next_byte_offset () + && other.get_start_byte_offset () < get_next_byte_offset ()) + { + byte_offset_t overlap_start = MAX (get_start_byte_offset (), + other.get_start_byte_offset ()); + byte_offset_t overlap_next = MIN (get_next_byte_offset (), + other.get_next_byte_offset ()); + gcc_assert (overlap_next > overlap_start); + *out_num_overlap_bytes = overlap_next - overlap_start; + return true; + } + else + return false; +} + +/* Return true if THIS exceeds OTHER and write the overhanging + byte range to OUT_OVERHANGING_BYTE_RANGE. */ + +bool +byte_range::exceeds_p (const byte_range &other, + byte_range *out_overhanging_byte_range) const +{ + if (other.get_last_byte_offset () < get_last_byte_offset ()) + { + /* THIS definitely exceeds OTHER. */ + byte_offset_t start = MAX (get_start_byte_offset (), + other.get_next_byte_offset ()); + byte_offset_t size = get_next_byte_offset () - start; + gcc_assert (size > 0); + out_overhanging_byte_range->m_start_byte_offset = start; + out_overhanging_byte_range->m_size_in_bytes = size; + return true; + } + else + return false; +} + +/* Return true if THIS falls short of OFFSET and write the + byte range fallen short to OUT_FALL_SHORT_BYTES. */ + +bool +byte_range::falls_short_of_p (byte_offset_t offset, + byte_range *out_fall_short_bytes) const +{ + if (get_start_byte_offset () < offset) + { + /* THIS falls short of OFFSET. */ + byte_offset_t start = get_start_byte_offset (); + byte_offset_t size = MIN (offset, get_next_byte_offset ()) - start; + gcc_assert (size > 0); + out_fall_short_bytes->m_start_byte_offset = start; + out_fall_short_bytes->m_size_in_bytes = size; + return true; + } + else + return false; +} + /* qsort comparator for byte ranges. */ int diff --git a/gcc/analyzer/store.h b/gcc/analyzer/store.h index 9b54c7b9156..ac8b6853f4b 100644 --- a/gcc/analyzer/store.h +++ b/gcc/analyzer/store.h @@ -310,6 +310,15 @@ struct byte_range && m_size_in_bytes == other.m_size_in_bytes); } + bool intersects_p (const byte_range &other, + byte_size_t *out_num_overlap_bytes) const; + + bool exceeds_p (const byte_range &other, + byte_range *out_overhanging_byte_range) const; + + bool falls_short_of_p (byte_offset_t offset, + byte_range *out_fall_short_bytes) const; + byte_offset_t get_start_byte_offset () const { return m_start_byte_offset; diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index fa23fbeaaaa..094135dbf74 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -457,6 +457,7 @@ Objective-C and Objective-C++ Dialects}. -Wno-analyzer-mismatching-deallocation @gol -Wno-analyzer-null-argument @gol -Wno-analyzer-null-dereference @gol +-Wno-analyzer-out-of-bounds @gol -Wno-analyzer-possible-null-argument @gol -Wno-analyzer-possible-null-dereference @gol -Wno-analyzer-shift-count-negative @gol @@ -9759,6 +9760,7 @@ Enabling this option effectively enables the following warnings: -Wanalyzer-mismatching-deallocation @gol -Wanalyzer-null-argument @gol -Wanalyzer-null-dereference @gol +-Wanalyzer-out-of-bounds @gol -Wanalyzer-possible-null-argument @gol -Wanalyzer-possible-null-dereference @gol -Wanalyzer-shift-count-negative @gol @@ -9951,6 +9953,19 @@ will warn about mismatches between @code{free}, scalar @code{delete} and vector @code{delete[]}, and those marked as allocator/deallocator pairs using attribute @code{malloc}. +@item -Wno-analyzer-out-of-bounds +@opindex Wanalyzer-out-of-bounds +@opindex Wno-analyzer-out-of-bounds +This warning requires @option{-fanalyzer} to enable it; use +@option{-Wno-analyzer-out-of-bounds} to disable it. + +This diagnostic warns for path through the code in which a buffer is +definitely read or written out-of-bounds. The diagnostic only applies +for cases where the analyzer is able to determine a constant offset and +for accesses past the end of a buffer, also a constant capacity. + +See @url{https://cwe.mitre.org/data/definitions/119.html, CWE-119: Improper Restriction of Operations within the Bounds of a Memory Buffer}. + @item -Wno-analyzer-possible-null-argument @opindex Wanalyzer-possible-null-argument @opindex Wno-analyzer-possible-null-argument diff --git a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C new file mode 100644 index 00000000000..d3076c3cf25 --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C @@ -0,0 +1,19 @@ +/* Reduced from pr100244.C. */ +inline void *operator new (__SIZE_TYPE__, void *__p) { return __p; } + +struct int_container { + int i; + int *addr () { return &i; } +}; + +struct int_and_addr { + int i; + int *addr; + int_and_addr () { addr = &i; } /* { dg-warning "overflow" } */ +}; + +int test (int_container ic) +{ + int_and_addr *iaddr = new (ic.addr ()) int_and_addr; + return iaddr->i; +} diff --git a/gcc/testsuite/g++.dg/analyzer/pr100244.C b/gcc/testsuite/g++.dg/analyzer/pr100244.C index 1d5e13dd865..6e2ac4c65bd 100644 --- a/gcc/testsuite/g++.dg/analyzer/pr100244.C +++ b/gcc/testsuite/g++.dg/analyzer/pr100244.C @@ -1,4 +1,7 @@ -// { dg-additional-options "-O1 -Wno-free-nonheap-object" } +// { dg-additional-options "-O1 -Wno-free-nonheap-object -Wno-analyzer-out-of-bounds" } +/* Disabled out-of-bounds checker because the output relied + on optimizations. out-of-bounds-placement-new.C tests + the same pattern but without optimizations. */ inline void *operator new (__SIZE_TYPE__, void *__p) { return __p; } diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c index 0c86f09f15b..9590e3137e0 100644 --- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c @@ -1,3 +1,5 @@ +/* { dg-additional-options -Wno-analyzer-out-of-bounds } */ + #include <stdlib.h> #include <stdio.h> #include <stdint.h> diff --git a/gcc/testsuite/gcc.dg/analyzer/memcpy-2.c b/gcc/testsuite/gcc.dg/analyzer/memcpy-2.c index 88ec84cb640..51e4a694951 100644 --- a/gcc/testsuite/gcc.dg/analyzer/memcpy-2.c +++ b/gcc/testsuite/gcc.dg/analyzer/memcpy-2.c @@ -1,4 +1,4 @@ -/* { dg-additional-options "-Wno-stringop-overflow" } */ +/* { dg-additional-options "-Wno-stringop-overflow -Wno-analyzer-out-of-bounds" } */ void main (int c, void *v) diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c new file mode 100644 index 00000000000..9f3cda6e02b --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c @@ -0,0 +1,120 @@ +#include <stdlib.h> +#include <string.h> +#include <stdint.h> +#include <stdio.h> + +/* Wanalyzer-out-of-bounds tests for buffer overflows. */ + +/* Avoid folding of memcpy. */ +typedef void * (*memcpy_t) (void *dst, const void *src, size_t n); + +static memcpy_t __attribute__((noinline)) +get_memcpy (void) +{ + return memcpy; +} + + +/* Taken from CWE-787. */ +void test1 (void) +{ + int id_sequence[3]; + + id_sequence[0] = 123; + id_sequence[1] = 234; + id_sequence[2] = 345; + id_sequence[3] = 456; /* { dg-line test1 } */ + + /* { dg-warning "overflow" "warning" { target *-*-* } test1 } */ + /* { dg-message "" "note" { target *-*-* } test1 } */ +} + +void test2 (void) +{ + int n = 4; + int arr[4]; + + for (int i = n - 1; i >= 0; i--) + arr[i] = i; +} + +void test3 (void) +{ + int n = 4; + int arr[4]; + + for (int i = n; i >= 0; i--) + arr[i] = i; /* { dg-line test3 } */ + + /* { dg-warning "overflow" "warning" { target *-*-* } test3 } */ + /* { dg-message "" "note" { target *-*-* } test3 } */ +} + +void test4 (void) +{ + int *arr = malloc (4 * sizeof (int)); + if (!arr) + return; + + int *last_el = arr + 3; + *last_el = 3; + + free (arr); +} + +void test5 (void) +{ + int *arr = malloc (4 * sizeof (int)); + if (!arr) + return; + + int *last_el = arr + 4; + *last_el = 4; /* { dg-line test5 } */ + + free (arr); + /* { dg-warning "overflow" "warning" { target *-*-* } test5 } */ + /* { dg-message "" "note" { target *-*-* } test5 } */ +} + +/* Taken from "A Provenance-aware Memory Object Model for C". */ +int y = 2, x = 1; /* { dg-message "capacity" } */ +void test6 (void) +{ + int *p = &x + 1; + int *q = &y; + printf ("Addresses: p=% p q=% p \n" , (void *) p, (void *) q); + if (memcmp (&p , &q , sizeof (p)) == 0) + { + *p = 11; /* { dg-line test6b } */ + printf ("x=%d y=%d *p=%d *q=%d\n" , x, y, *p, *q); /* { dg-line test6c } */ + } + + /* { dg-warning "overflow" "warning" { target *-*-* } test6b } */ + /* { dg-message "" "note" { target *-*-* } test6b } */ + /* { dg-warning "overread" "warning" { target *-*-* } test6c } */ + /* { dg-message "" "note" { target *-*-* } test6c } */ +} + +extern int is_valid (void); + +int returnChunkSize (void *ptr) +{ + /* If chunk info is valid, return the size of usable memory, + else, return -1 to indicate an error. */ + return is_valid () ? sizeof (*ptr) : -1; +} + +/* Taken from CWE-787. */ +void test7 (void) +{ + memcpy_t fn = get_memcpy (); + + int destBuf[4]; + int srcBuf[4]; + fn (destBuf, srcBuf, returnChunkSize (destBuf)); /* { dg-line test7 } */ + + // TODO: Should we handle widening_svalues as a follow-up? + /* { dg-warning "overread" "warning" { xfail *-*-* } test7 } */ + /* { dg-warning "overflow" "warning" { xfail *-*-* } test7 } */ + /* { dg-message "" "note" { xfail *-*-* } test7 } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c new file mode 100644 index 00000000000..0df9364c5c1 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c @@ -0,0 +1,83 @@ +#include <stdlib.h> +#include <string.h> +#include <stdio.h> +#include <stdint.h> + +/* Wanalyzer-out-of-bounds tests for buffer overreads. */ + +/* Avoid folding of memcpy. */ +typedef void * (*memcpy_t) (void *dst, const void *src, size_t n); + +static memcpy_t __attribute__((noinline)) +get_memcpy (void) +{ + return memcpy; +} + + +void test1 (void) +{ + int id_sequence[3]; + memset (id_sequence, 0, 3 * sizeof(int)); + printf ("%i", id_sequence[3]); /* { dg-line test1 } */ + + /* { dg-warning "overread" "warning" { target *-*-* } test1 } */ + /* { dg-message "" "note" { target *-*-* } test1 } */ +} + +void test2 (void) +{ + int n = 4; + int arr[n]; + memset (arr, 0, n * sizeof (int)); + + int sum = 0; + for (int i = n - 1; i >= 0; i--) + sum += arr[i]; +} + +void test3 (void) +{ + int n = 4; + int arr[4]; + memset (arr, 0, n * sizeof (int)); + + int sum = 0; + for (int i = n; i > 0; i--) + sum += arr[i]; /* { dg-line test3 } */ + + /* { dg-warning "overread" "warning" { target *-*-* } test3 } */ + /* { dg-message "" "note" { target *-*-* } test3 } */ +} + +void test4 (void) +{ + int n = 4; + int *arr = malloc (n * sizeof (int)); + if (!arr) + return; + memset (arr, 0, n * sizeof(int)); + + int sum = 0; + for (int i = n - 1; i >= 0; i--) + sum += *(arr + i); + + free (arr); +} + +void test5 (void) +{ + int n = 4; + int *arr = malloc (n * sizeof (int)); + if (!arr) + return; + memset (arr, 0, n * sizeof(int)); + + int sum = 0; + for (int i = n; i > 0; i--) + sum += *(arr + i); /* { dg-line test5 } */ + + free (arr); + /* { dg-warning "overread" "warning" { target *-*-* } test5 } */ + /* { dg-message "" "note" { target *-*-* } test5 } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-3.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-3.c new file mode 100644 index 00000000000..7446b182e48 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-3.c @@ -0,0 +1,91 @@ +#include <stdlib.h> +#include <string.h> +#include <stdint.h> + +/* Wanalyzer-out-of-bounds tests for buffer underreads and writes. */ + +/* Avoid folding of memcpy. */ +typedef void * (*memcpy_t) (void *dst, const void *src, size_t n); +static memcpy_t __attribute__((noinline)) +get_memcpy (void) +{ + return memcpy; +} + + +void test1 (void) +{ + int buf[4]; + int *e = buf - 1; + *e = 42; /* { dg-line test1 } */ + + /* { dg-warning "underflow" "warning" { target *-*-* } test1 } */ + /* { dg-message "" "note" { target *-*-* } test1 } */ +} + +void test2 (void) +{ + int buf[4]; + int *e = buf + 1; + *e = 123; + *(e - 1) = 321; +} + +void test3 (void) +{ + int buf[4]; + int *e = buf + 1; + *e = 123; + *(e - 2) = 321; /* { dg-line test3 } */ + + /* { dg-warning "underflow" "warning" { target *-*-* } test3 } */ + /* { dg-message "" "note" { target *-*-* } test3 } */ +} + +void test4 (void) +{ + memcpy_t fn = get_memcpy (); + int buf[4]; + memset (buf, 1, 4 * sizeof (int)); + int n = -4; + fn (&(buf[n]), buf, sizeof (int)); /* { dg-line test4 } */ + + /* { dg-warning "underflow" "warning" { target *-*-* } test4 } */ + /* { dg-message "" "note" { target *-*-* } test4 } */ +} + +void test5 (void) +{ + int buf[4]; + memset (buf, 1, 4 * sizeof (int)); + + int sum = 0; + for (int i = 4; i >= 0; i++) + sum += *(buf - i); /* { dg-line test5 } */ + + /* { dg-warning "underread" "warning" { target *-*-* } test5 } */ + /* { dg-message "" "note" { target *-*-* } test5 } */ +} + +void test6 (void) +{ + int buf[4]; + memset (buf, 1, 4 * sizeof (int)); + + int *view = buf + 1; + int sum = 0; + for (int i = 0; i < 4; i++) + sum += *(view++); +} + +void test8 (void) +{ + memcpy_t fn = get_memcpy (); + int buf[4]; + memset (buf, 1, 4 * sizeof (int)); + int n = -4; + fn (buf, &(buf[n]), sizeof (int)); /* { dg-line test8 } */ + + /* { dg-warning "underread" "warning" { target *-*-* } test8 } */ + /* { dg-message "" "note" { target *-*-* } test8 } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-container_of.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-container_of.c new file mode 100644 index 00000000000..172ec474c42 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-container_of.c @@ -0,0 +1,51 @@ +/* Further reduced container_of pattern from the Linux Kernel. */ + +struct inner { + /* Don't care */ +}; + +struct outer { + int i; + struct inner inner_struct; +}; + +struct outer *container_of (struct inner *ptr_to_inner) +{ + struct outer *ptr_to_outer = ((struct outer *) (((void *) ptr_to_inner) - __builtin_offsetof(struct outer, inner_struct))); + return ptr_to_outer; +} + +int test (struct outer *outer_p, struct inner *inner_p) +{ + struct outer test; + test.i = 42; + struct inner test2; + int sum = 0; + struct outer *o; + + /* Symbolic inner struct. */ + o = container_of (inner_p); + sum += o->i; // ok + /* Not ok, but we can't be sure that outer + is actually the container of inner. */ + sum += (o - 1)->i; + /* Symbolic outer struct. */ + o = container_of (&(outer_p->inner_struct)); + sum += o->i; // ok + /* Not ok, but indistinguishable from the case above. */ + sum += (o - 1)->i; + /* Concrete outer struct. */ + o = container_of (&(test.inner_struct)); + sum += o->i; // ok + /* Not ok and we do have a concrete region. */ + sum += (o - 1)->i; /* { dg-line testA } */ + /* Concrete inner struct, has no container. */ + o = container_of (&test2); + sum += o->i; /* { dg-line testB } */ + + return sum; + /* { dg-warning "underread" "warning" { target *-*-* } testA } */ + /* { dg-message "" "note" { target *-*-* } testA } */ + /* { dg-warning "underread" "warning" { target *-*-* } testB } */ + /* { dg-message "" "note" { target *-*-* } testB } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-coreutils.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-coreutils.c new file mode 100644 index 00000000000..cd0f4b7f7b2 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-coreutils.c @@ -0,0 +1,29 @@ +/* Reduced from coreutils/ls.c attach. */ + +void add_zero_terminator (char *buf) +{ + char *end = buf; + while (end++); + if (buf < end) + end[-1] = '\0'; +} + +/* Reduced from coreutils/cat.c. */ + +#define LINE_COUNTER_BUF_LEN 20 +static char line_buf[LINE_COUNTER_BUF_LEN] = + { + ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', + ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', '0', + '\t', '\0' + }; + +/* Position of the first digit in 'line_buf'. */ +static char *line_num_start = line_buf + LINE_COUNTER_BUF_LEN - 3; + +static void +next_line_num (void) +{ + if (line_num_start > line_buf) + *--line_num_start = '1'; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-curl.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-curl.c new file mode 100644 index 00000000000..e34b572966e --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-curl.c @@ -0,0 +1,41 @@ +/* { dg-additional-options "-O2" } */ +#include <string.h> + +/* Reduced from curl lib/smb.c. */ +typedef int CURLcode; + +struct smb_conn { + // [...] + char *user; +}; + +struct smb_setup { + // [...] + char bytes[48]; +} __attribute__((packed)); + +struct connectdata { + // [...] + struct smb_conn *smbc; +}; + +CURLcode smb_send_setup (struct connectdata *conn) +{ + struct smb_conn *smbc = conn->smbc; + struct smb_setup msg; + char *p = msg.bytes; + unsigned char lm[24]; + + /* Init to prevent uninit warning. */ + memset(&msg, 0, sizeof(msg)); + memset (&lm, 0, sizeof(lm)); + + memcpy(p, lm, sizeof(lm)); + p += sizeof(lm); + /* Had a false-positive overflow at p. Checker had a number of bytes copied + relative to the start but offset points in the middle the field. */ + strcpy(p, (smbc->user)); + p += strlen(smbc->user) + 1; + + return 1; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr101962.c b/gcc/testsuite/gcc.dg/analyzer/pr101962.c index d15820af27f..cf0041b2fcd 100644 --- a/gcc/testsuite/gcc.dg/analyzer/pr101962.c +++ b/gcc/testsuite/gcc.dg/analyzer/pr101962.c @@ -22,8 +22,10 @@ test_1 (void) a = maybe_inc_int_ptr (a); __analyzer_eval (a == NULL); /* { dg-warning "FALSE" } */ __analyzer_eval (a != NULL); /* { dg-warning "TRUE" } */ - return *a; /* { dg-warning "use of uninitialized value '\\*a'" } */ - /* TODO: a complaint about out-of-bounds would be a better warning. */ + return *a; /* { dg-line test_1 } */ + + /* { dg-warning "use of uninitialized value '\\*a'" "warning" { target *-*-* } test_1 } */ + /* { dg-warning "overread" "warning" { target *-*-* } test_1 } */ } static const char * __attribute__((noinline)) diff --git a/gcc/testsuite/gcc.dg/analyzer/pr96764.c b/gcc/testsuite/gcc.dg/analyzer/pr96764.c index 70b25d3ab81..6024ebaaa2a 100644 --- a/gcc/testsuite/gcc.dg/analyzer/pr96764.c +++ b/gcc/testsuite/gcc.dg/analyzer/pr96764.c @@ -1,3 +1,5 @@ +/* { dg-additional-options "-Wno-analyzer-out-of-bounds" } */ + void ar (int *hd) { diff --git a/gcc/testsuite/gcc.dg/analyzer/pr97029.c b/gcc/testsuite/gcc.dg/analyzer/pr97029.c index ff83ad4d56e..e3b45311779 100644 --- a/gcc/testsuite/gcc.dg/analyzer/pr97029.c +++ b/gcc/testsuite/gcc.dg/analyzer/pr97029.c @@ -1,4 +1,6 @@ -struct vj {}; +struct vj { + char buf[1]; +}; void setjmp (struct vj pl) diff --git a/gcc/testsuite/gcc.dg/analyzer/test-setjmp.h b/gcc/testsuite/gcc.dg/analyzer/test-setjmp.h index db2422709e2..52c57d02b70 100644 --- a/gcc/testsuite/gcc.dg/analyzer/test-setjmp.h +++ b/gcc/testsuite/gcc.dg/analyzer/test-setjmp.h @@ -12,7 +12,9 @@ #pragma GCC system_header -struct __jmp_buf_tag {}; +struct __jmp_buf_tag { + char buf[1]; +}; typedef struct __jmp_buf_tag jmp_buf[1]; typedef struct __jmp_buf_tag sigjmp_buf[1]; diff --git a/gcc/testsuite/gcc.dg/analyzer/zlib-3.c b/gcc/testsuite/gcc.dg/analyzer/zlib-3.c index 5098b4fc363..b05b8629a7f 100644 --- a/gcc/testsuite/gcc.dg/analyzer/zlib-3.c +++ b/gcc/testsuite/gcc.dg/analyzer/zlib-3.c @@ -182,7 +182,9 @@ static int huft_build(uInt *b, uInt n, uInt s, const uInt *d, const uInt *e, q[j] = r; /* { dg-warning "use of uninitialized value 'r.base'" } */ mask = (1 << w) - 1; - while ((i & mask) != x[h]) { + /* The analyzer thinks that h can be -1 here. + This is probably a false positive. */ + while ((i & mask) != x[h]) { /* { dg-bogus "underread" "" { xfail *-*-* } } */ h--; w -= l; mask = (1 << w) - 1; -- 2.37.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2 v2] analyzer: out-of-bounds checker [PR106000] 2022-08-11 17:24 ` [PATCH 2/2 v2] analyzer: out-of-bounds checker [PR106000] Tim Lange @ 2022-08-11 19:30 ` David Malcolm 0 siblings, 0 replies; 8+ messages in thread From: David Malcolm @ 2022-08-11 19:30 UTC (permalink / raw) To: Tim Lange, gcc-patches On Thu, 2022-08-11 at 19:24 +0200, Tim Lange wrote: > This patch adds an experimental out-of-bounds checker to the > analyzer. > > The checker was tested on coreutils, curl, httpd and openssh. It is > mostly > accurate but does produce false-positives on yacc-generated files and > sometimes when the analyzer misses an invariant. These cases will be > documented in bugzilla. > Regression-tested on Linux x86-64, further ran the analyzer tests > with > the -m32 option. [...snip...] Thanks for the updated patch. This version is OK for trunk [1]. Dave [1] as I mentioned before, I plan to reword the diagnostic once it's in trunk, but I'll do that myself, rather than be a backseat driver. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2 v2] analyzer: consider that realloc could shrink the buffer [PR106539] 2022-08-11 17:24 ` [PATCH 1/2 v2] " Tim Lange 2022-08-11 17:24 ` [PATCH 2/2 v2] analyzer: out-of-bounds checker [PR106000] Tim Lange @ 2022-08-11 19:25 ` David Malcolm 1 sibling, 0 replies; 8+ messages in thread From: David Malcolm @ 2022-08-11 19:25 UTC (permalink / raw) To: Tim Lange, gcc-patches 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); > +} ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-08-11 19:30 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-09 21:19 [PATCH 1/2] analyzer: consider that realloc could shrink the buffer [PR106539] Tim Lange 2022-08-09 21:19 ` [PATCH 2/2] analyzer: out-of-bounds checker [PR106000] Tim Lange 2022-08-09 22:44 ` David Malcolm 2022-08-09 22:02 ` [PATCH 1/2] analyzer: consider that realloc could shrink the buffer [PR106539] David Malcolm 2022-08-11 17:24 ` [PATCH 1/2 v2] " Tim Lange 2022-08-11 17:24 ` [PATCH 2/2 v2] analyzer: out-of-bounds checker [PR106000] Tim Lange 2022-08-11 19:30 ` David Malcolm 2022-08-11 19:25 ` [PATCH 1/2 v2] analyzer: consider that realloc could shrink the buffer [PR106539] David Malcolm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).