From: David Malcolm <dmalcolm@redhat.com>
To: gcc-patches@gcc.gnu.org
Cc: Tim Lange <mail@tim-lange.me>, David Malcolm <dmalcolm@redhat.com>
Subject: [PATCH 02/21] analyzer: Fix allocation size false positive on conjured svalue [PR109577]
Date: Thu, 9 May 2024 13:42:17 -0400 [thread overview]
Message-ID: <20240509174236.2278921-3-dmalcolm@redhat.com> (raw)
In-Reply-To: <20240509174236.2278921-1-dmalcolm@redhat.com>
From: Tim Lange <mail@tim-lange.me>
Currently, the analyzer tries to prove that the allocation size is a
multiple of the pointee's type size. This patch reverses the behavior
to try to prove that the expression is not a multiple of the pointee's
type size. With this change, each unhandled case should be gracefully
considered as correct. This fixes the bug reported in PR 109577 by
Paul Eggert.
Regression-tested on Linux x86-64 with -m32 and -m64.
2023-06-09 Tim Lange <mail@tim-lange.me>
PR analyzer/109577
gcc/analyzer/ChangeLog:
* constraint-manager.cc (class sval_finder): Visitor to find
childs in svalue trees.
(constraint_manager::sval_constrained_p): Add new function to
check whether a sval might be part of an constraint.
* constraint-manager.h: Add sval_constrained_p function.
* region-model.cc (class size_visitor): Reverse behavior to not
emit a warning on not explicitly considered cases.
(region_model::check_region_size):
Adapt to size_visitor changes.
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/allocation-size-2.c: Change expected output
and add new test case.
* gcc.dg/analyzer/pr109577.c: New test.
(cherry picked from commit r14-1684-g1d57a2232575913ad1085bac0ba5e22b58185179)
Signed-off-by: David Malcolm <dmalcolm@redhat.com>
---
gcc/analyzer/constraint-manager.cc | 131 ++++++++++++++++++
gcc/analyzer/constraint-manager.h | 1 +
gcc/analyzer/region-model.cc | 80 ++++-------
.../gcc.dg/analyzer/allocation-size-2.c | 24 ++--
gcc/testsuite/gcc.dg/analyzer/pr109577.c | 16 +++
5 files changed, 194 insertions(+), 58 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr109577.c
diff --git a/gcc/analyzer/constraint-manager.cc b/gcc/analyzer/constraint-manager.cc
index 2c9c435527e..9211366fb7c 100644
--- a/gcc/analyzer/constraint-manager.cc
+++ b/gcc/analyzer/constraint-manager.cc
@@ -2218,6 +2218,137 @@ constraint_manager::get_equiv_class_by_svalue (const svalue *sval,
return false;
}
+/* Tries to find a svalue inside another svalue. */
+
+class sval_finder : public visitor
+{
+public:
+ sval_finder (const svalue *query) : m_query (query), m_found (false)
+ {
+ }
+
+ bool found_query_p ()
+ {
+ return m_found;
+ }
+
+ void visit_region_svalue (const region_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_constant_svalue (const constant_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_unknown_svalue (const unknown_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_poisoned_svalue (const poisoned_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_setjmp_svalue (const setjmp_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_initial_svalue (const initial_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_unaryop_svalue (const unaryop_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_binop_svalue (const binop_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_sub_svalue (const sub_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_repeated_svalue (const repeated_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_bits_within_svalue (const bits_within_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_unmergeable_svalue (const unmergeable_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_placeholder_svalue (const placeholder_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_widening_svalue (const widening_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_compound_svalue (const compound_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_conjured_svalue (const conjured_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_asm_output_svalue (const asm_output_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+ void visit_const_fn_result_svalue (const const_fn_result_svalue *sval)
+ {
+ m_found |= m_query == sval;
+ }
+
+private:
+ const svalue *m_query;
+ bool m_found;
+};
+
+/* Returns true if SVAL is constrained. */
+
+bool
+constraint_manager::sval_constrained_p (const svalue *sval) const
+{
+ int i;
+ equiv_class *ec;
+ sval_finder finder (sval);
+ FOR_EACH_VEC_ELT (m_equiv_classes, i, ec)
+ {
+ int j;
+ const svalue *iv;
+ FOR_EACH_VEC_ELT (ec->m_vars, j, iv)
+ {
+ iv->accept (&finder);
+ if (finder.found_query_p ())
+ return true;
+ }
+ }
+ return false;
+}
+
/* Ensure that SVAL has an equivalence class within this constraint_manager;
return the ID of the class. */
diff --git a/gcc/analyzer/constraint-manager.h b/gcc/analyzer/constraint-manager.h
index 3afbc7f848e..72753e43c96 100644
--- a/gcc/analyzer/constraint-manager.h
+++ b/gcc/analyzer/constraint-manager.h
@@ -459,6 +459,7 @@ public:
bool get_equiv_class_by_svalue (const svalue *sval,
equiv_class_id *out) const;
+ bool sval_constrained_p (const svalue *sval) const;
equiv_class_id get_or_add_equiv_class (const svalue *sval);
tristate eval_condition (equiv_class_id lhs,
enum tree_code op,
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 18996c5e5e8..c98b09d5322 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -2949,7 +2949,7 @@ capacity_compatible_with_type (tree cst, tree pointee_size_tree)
It works by visiting all svalues inside SVAL until it reaches
atomic nodes. From those, it goes back up again and adds each
- node that might be a multiple of SIZE_CST to the RESULT_SET. */
+ node that is not a multiple of SIZE_CST to the RESULT_SET. */
class size_visitor : public visitor
{
@@ -2960,7 +2960,7 @@ public:
m_root_sval->accept (this);
}
- bool get_result ()
+ bool is_dubious_capacity ()
{
return result_set.contains (m_root_sval);
}
@@ -2970,22 +2970,10 @@ public:
check_constant (sval->get_constant (), sval);
}
- void visit_unknown_svalue (const unknown_svalue *sval ATTRIBUTE_UNUSED)
- final override
- {
- result_set.add (sval);
- }
-
- void visit_poisoned_svalue (const poisoned_svalue *sval ATTRIBUTE_UNUSED)
- final override
- {
- result_set.add (sval);
- }
-
void visit_unaryop_svalue (const unaryop_svalue *sval) final override
{
- const svalue *arg = sval->get_arg ();
- if (result_set.contains (arg))
+ if (CONVERT_EXPR_CODE_P (sval->get_op ())
+ && result_set.contains (sval->get_arg ()))
result_set.add (sval);
}
@@ -2994,28 +2982,24 @@ public:
const svalue *arg0 = sval->get_arg0 ();
const svalue *arg1 = sval->get_arg1 ();
- if (sval->get_op () == MULT_EXPR)
- {
- if (result_set.contains (arg0) || result_set.contains (arg1))
- result_set.add (sval);
- }
- else
+ switch (sval->get_op ())
{
- if (result_set.contains (arg0) && result_set.contains (arg1))
- result_set.add (sval);
+ case MULT_EXPR:
+ if (result_set.contains (arg0) && result_set.contains (arg1))
+ result_set.add (sval);
+ break;
+ case PLUS_EXPR:
+ case MINUS_EXPR:
+ if (result_set.contains (arg0) || result_set.contains (arg1))
+ result_set.add (sval);
+ break;
+ default:
+ break;
}
}
- void visit_repeated_svalue (const repeated_svalue *sval) final override
- {
- sval->get_inner_svalue ()->accept (this);
- if (result_set.contains (sval->get_inner_svalue ()))
- result_set.add (sval);
- }
-
void visit_unmergeable_svalue (const unmergeable_svalue *sval) final override
{
- sval->get_arg ()->accept (this);
if (result_set.contains (sval->get_arg ()))
result_set.add (sval);
}
@@ -3025,33 +3009,30 @@ public:
const svalue *base = sval->get_base_svalue ();
const svalue *iter = sval->get_iter_svalue ();
- if (result_set.contains (base) && result_set.contains (iter))
+ if (result_set.contains (base) || result_set.contains (iter))
result_set.add (sval);
}
- void visit_conjured_svalue (const conjured_svalue *sval ATTRIBUTE_UNUSED)
- final override
+ void visit_initial_svalue (const initial_svalue *sval) final override
{
- equiv_class_id id (-1);
+ equiv_class_id id = equiv_class_id::null ();
if (m_cm->get_equiv_class_by_svalue (sval, &id))
{
if (tree cst = id.get_obj (*m_cm).get_any_constant ())
check_constant (cst, sval);
- else
- result_set.add (sval);
+ }
+ else if (!m_cm->sval_constrained_p (sval))
+ {
+ result_set.add (sval);
}
}
- void visit_asm_output_svalue (const asm_output_svalue *sval ATTRIBUTE_UNUSED)
- final override
- {
- result_set.add (sval);
- }
-
- void visit_const_fn_result_svalue (const const_fn_result_svalue
- *sval ATTRIBUTE_UNUSED) final override
+ void visit_conjured_svalue (const conjured_svalue *sval) final override
{
- result_set.add (sval);
+ equiv_class_id id = equiv_class_id::null ();
+ if (m_cm->get_equiv_class_by_svalue (sval, &id))
+ if (tree cst = id.get_obj (*m_cm).get_any_constant ())
+ check_constant (cst, sval);
}
private:
@@ -3061,10 +3042,9 @@ private:
{
default:
/* Assume all unhandled operands are compatible. */
- result_set.add (sval);
break;
case INTEGER_CST:
- if (capacity_compatible_with_type (cst, m_size_cst))
+ if (!capacity_compatible_with_type (cst, m_size_cst))
result_set.add (sval);
break;
}
@@ -3187,7 +3167,7 @@ region_model::check_region_size (const region *lhs_reg, const svalue *rhs_sval,
if (!is_struct)
{
size_visitor v (pointee_size_tree, capacity, m_constraints);
- if (!v.get_result ())
+ if (v.is_dubious_capacity ())
{
tree expr = get_representative_tree (capacity);
ctxt->warn (make_unique <dubious_allocation_size> (lhs_reg,
diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
index 2cf64e97b41..eb770f73d4a 100644
--- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
+++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
@@ -76,13 +76,13 @@ void *create_buffer(int32_t n)
return malloc(n);
}
-void test_7(int32_t n)
+void test_7(int32_t n)
{
int32_t *buf = create_buffer(n * sizeof (int32_t));
free (buf);
}
-void test_8(int32_t n)
+void test_8(int32_t n)
{
/* FIXME: At the moment, region_model::set_value (lhs, <return_value>)
is called at the src_node of the return edge. This edge has no stmts
@@ -98,13 +98,11 @@ void test_9 (void)
{
int32_t n;
scanf("%i", &n);
- /* n is a conjured_svalue. */
- void *ptr = malloc (n); /* { dg-message "'n' bytes" "note" } */
- int32_t *iptr = (int32_t *)ptr; /* { dg-line assign9 } */
+ /* n is a conjured_svalue without any constraint. We have to assume
+ that is a multiple of sizeof (int32_t *); see PR analyzer/110014. */
+ void *ptr = malloc (n);
+ int32_t *iptr = (int32_t *)ptr;
free (iptr);
-
- /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } assign9 } */
- /* { dg-message "'int32_t \\*' (\\\{aka '(long )?int \\*'\\\})? here; 'sizeof \\(int32_t (\\\{aka (long )?int\\\})?\\)' is '4'" "note" { target *-*-* } assign9 } */
}
void test_11 (void)
@@ -157,3 +155,13 @@ void test_13 (void)
else
free (ptr);
}
+
+int *test_14 (size_t n)
+{
+ int *ptr = NULL;
+ /* n is an initial_svalue and guarded such that there is no equiv_class
+ for n itself but only for a binop_svalue containing n. */
+ if (n % sizeof (int) == 0)
+ ptr = malloc (n);
+ return ptr;
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr109577.c b/gcc/testsuite/gcc.dg/analyzer/pr109577.c
new file mode 100644
index 00000000000..a6af6f7019f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr109577.c
@@ -0,0 +1,16 @@
+void *malloc (unsigned long);
+
+double *
+unsafe (unsigned long n)
+{
+ return malloc (n * sizeof (double));
+}
+
+double *
+safer (unsigned long n)
+{
+ unsigned long nbytes;
+ if (__builtin_mul_overflow (n, sizeof (double), &nbytes))
+ return 0;
+ return malloc (nbytes);
+}
--
2.26.3
next prev parent reply other threads:[~2024-05-09 17:42 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-09 17:42 [pushed 00/21] Various backports to gcc 13 (analyzer, jit, diagnostics) David Malcolm
2024-05-09 17:42 ` [PATCH 01/21] analyzer: add caching to globals with initializers [PR110112] David Malcolm
2024-05-09 17:42 ` David Malcolm [this message]
2024-05-11 16:43 ` [PATCH 02/21] analyzer: Fix allocation size false positive on conjured svalue [PR109577] NightStrike
2024-05-09 17:42 ` [PATCH 03/21] testsuite: Add more allocation size tests for conjured svalues [PR110014] David Malcolm
2024-05-11 16:44 ` NightStrike
2024-05-09 17:42 ` [PATCH 04/21] jit: avoid using __vector in testcase [PR110466] David Malcolm
2024-05-09 17:42 ` [PATCH 05/21] jit.exp: handle dwarf version mismatch in jit-check-debug-info [PR110466] David Malcolm
2024-05-09 17:42 ` [PATCH 06/21] analyzer: fix ICE on division of tainted floating-point values [PR110700] David Malcolm
2024-05-09 17:42 ` [PATCH 07/21] analyzer: fix ICE on zero-sized arrays [PR110882] David Malcolm
2024-05-09 17:42 ` [PATCH 08/21] testsuite, analyzer: add test case [PR108171] David Malcolm
2024-05-09 17:42 ` [PATCH 09/21] jit: dump string literal initializers correctly David Malcolm
2024-05-09 17:42 ` [PATCH 10/21] analyzer: fix ICE for 2 bits before the start of base region [PR112889] David Malcolm
2024-05-09 17:42 ` [PATCH 11/21] analyzer: fix deref-before-check false positives due to inlining [PR112790] David Malcolm
2024-05-09 17:42 ` [PATCH 12/21] analyzer: casting all zeroes should give all zeroes [PR113333] David Malcolm
2024-05-09 17:42 ` [PATCH 13/21] analyzer: fix defaults in compound assignments from non-zero offsets [PR112969] David Malcolm
2024-05-09 17:42 ` [PATCH 14/21] analyzer: fix skipping of debug stmts [PR113253] David Malcolm
2024-05-09 17:42 ` [PATCH 15/21] analyzer: fix -Wanalyzer-va-arg-type-mismatch false +ve on int types [PR111289] David Malcolm
2024-05-09 17:42 ` [PATCH 16/21] analyzer: fix -Wanalyzer-deref-before-check false positive seen in loop header macro [PR109251] David Malcolm
2024-05-09 17:42 ` [PATCH 17/21] analyzer: fix ICE due to type mismatch when replaying call summary [PR114473] David Malcolm
2024-05-09 17:42 ` [PATCH 18/21] analyzer: fix ICE and false positive with -Wanalyzer-deref-before-check [PR114408] David Malcolm
2024-05-09 17:42 ` [PATCH 19/21] diagnostics: fix ICE on sarif output when source file is unreadable [PR111700] David Malcolm
2024-05-09 17:42 ` [PATCH 20/21] Fix ICE in -fdiagnostics-generate-patch [PR112684] David Malcolm
2024-05-09 17:42 ` [PATCH 21/21] diagnostics: fix corrupt json/SARIF on stderr [PR114348] David Malcolm
2024-05-13 9:03 ` [pushed 00/21] Various backports to gcc 13 (analyzer, jit, diagnostics) Jakub Jelinek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240509174236.2278921-3-dmalcolm@redhat.com \
--to=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=mail@tim-lange.me \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).