[PATCH 1/2] analyzer: Fix allocation size false positive on conjured svalue [PR109577]
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
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.
---
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..24cd8960098 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)
+ {
+ }
+
+ 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
[PATCH 2/2] testsuite: Add more allocation size tests for conjured svalues [PR110014]
This patch adds the reproducers reported in PR 110014 as test cases. The
false positives in those cases are already fixed with PR 109577.
2023-06-09 Tim Lange
PR analyzer/110014
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/pr110014.c: New tests.
---
gcc/testsuite/gcc.dg/analyzer/pr110014.c | 25
1 file changed, 25 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr110014.c
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr110014.c
b/gcc/testsuite/gcc.dg/analyzer/pr110014.c
new file mode 100644
index 000..d76b8781413
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr110014.c
@@ -0,0 +1,25 @@
+void *realloc (void *, unsigned long)
+ __attribute__((__nothrow__, __leaf__))
+ __attribute__((__warn_unused_result__)) __attribute__((__alloc_size__ (2)));
+
+long *
+slurp (long *buffer, unsigned long file_size)
+{
+ unsigned long cc;
+ if (!__builtin_add_overflow (file_size - file_size % sizeof (long),
+ 2 * sizeof (long), &cc))
+buffer = realloc (buffer, cc);
+ return buffer;
+}
+
+long *
+slurp1 (long *buffer, unsigned long file_size)
+{
+ return realloc (buffer, file_size - file_size % sizeof (long));
+}
+
+long *
+slurp2 (long *buffer, unsigned long file_size)
+{
+ return realloc (buffer, (file_size / sizeof (long)) * sizeof (long));
+}
--
2.40.1
[PATCH] MAINTAINERS: Add myself to write after approval and DCO
Hi everyone, I've added myself to write after approval and DCO section. - Tim 2022-07-02 Tim Lange ChangeLog: * MAINTAINERS: Add myself. --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 3c448ba9eb6..17bebefa2db 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -495,6 +495,7 @@ Razya Ladelsky Thierry Lafage Rask Ingemann Lambertsen Jerome Lambourg +Tim Lange Asher Langton Chris Lattner Terry Laurenzo @@ -716,6 +717,7 @@ information. Matthias Kretz +Tim Lange Jeff Law Jeff Law Gaius Mulley -- 2.36.1
[PATCH] analyzer: Use fixed-width types in allocation size tests
Hi,
thanks for the mail! Embarrassingly, I did not account for the different
sizes types may have on different systems. I've switched all testcases
to the fixed-width types of stdint.h.
Does this patch need an approval?
- Tim
The patch changes the types inside the tests for the allocation size
checker to fixed-width types of stdint.h to account for different
architectures with different type widths.
2022-07-03 Tim Lange
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/allocation-size-1.c: Use fixed-length types.
* gcc.dg/analyzer/allocation-size-2.c: Likewise.
* gcc.dg/analyzer/allocation-size-3.c: Likewise.
* gcc.dg/analyzer/allocation-size-4.c: Likewise.
* gcc.dg/analyzer/allocation-size-5.c: Likewise.
---
.../gcc.dg/analyzer/allocation-size-1.c | 53 +++---
.../gcc.dg/analyzer/allocation-size-2.c | 73 ++-
.../gcc.dg/analyzer/allocation-size-3.c | 19 ++---
.../gcc.dg/analyzer/allocation-size-4.c | 13 ++--
.../gcc.dg/analyzer/allocation-size-5.c | 23 +++---
5 files changed, 93 insertions(+), 88 deletions(-)
diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
index 4fc2bf75d6c..e6d021a128f 100644
--- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
@@ -1,79 +1,80 @@
#include
#include
+#include
/* Tests with constant buffer sizes. */
void test_1 (void)
{
- short *ptr = malloc (21 * sizeof (short));
+ int16_t *ptr = malloc (21 * sizeof (int16_t));
free (ptr);
}
void test_2 (void)
{
- int *ptr = malloc (21 * sizeof (short)); /* { dg-line malloc2 } */
+ int32_t *ptr = malloc (21 * sizeof (int16_t)); /* { dg-line malloc2 } */
free (ptr);
/* { dg-warning "allocated buffer size is not a multiple of the pointee's
size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc2 } */
/* { dg-message "\\d+ bytes" "note" { target *-*-* } malloc2 } */
- /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" {
target *-*-* } malloc2 } */
+ /* { dg-message "'int32_t \\*' (\\\{aka 'int \\*'\\\})? here; 'sizeof
\\(int32_t (\\\{aka int\\\})\\)' is '\\d+'" "note" { target *-*-* } malloc2 } */
}
void test_3 (void)
{
- void *ptr = malloc (21 * sizeof (short));
- short *sptr = (short *)ptr;
+ void *ptr = malloc (21 * sizeof (int16_t));
+ int16_t *sptr = (int16_t *)ptr;
free (sptr);
}
void test_4 (void)
{
- void *ptr = malloc (21 * sizeof (short)); /* { dg-message "\\d+ bytes" } */
- int *iptr = (int *)ptr; /* { dg-line assign4 } */
+ void *ptr = malloc (21 * sizeof (int16_t)); /* { dg-message "\\d+ bytes" } */
+ int32_t *iptr = (int32_t *)ptr; /* { dg-line assign4 } */
free (iptr);
/* { dg-warning "allocated buffer size is not a multiple of the pointee's
size \\\[CWE-131\\\]" "warning" { target *-*-* } assign4 } */
- /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" {
target *-*-* } assign4 } */
+ /* { dg-message "'int32_t \\*' (\\\{aka 'int \\*'\\\})? here; 'sizeof
\\(int32_t (\\\{aka int\\\})\\)' is '\\d+'" "note" { target *-*-* } assign4 } */
}
void test_5 (void)
{
- int user_input;
+ int32_t user_input;
scanf("%i", &user_input);
- int n;
+ int32_t n;
if (user_input == 0)
-n = 21 * sizeof (short);
+n = 21 * sizeof (int16_t);
else
-n = 42 * sizeof (short);
+n = 42 * sizeof (int16_t);
void *ptr = malloc (n);
- short *sptr = (short *)ptr;
+ int16_t *sptr = (int16_t *)ptr;
free (sptr);
}
void test_6 (void)
{
- int user_input;
+ int32_t user_input;
scanf("%i", &user_input);
- int n;
+ int32_t n;
if (user_input == 0)
-n = 21 * sizeof (short);
+n = 21 * sizeof (int16_t);
else
-n = 42 * sizeof (short);
+n = 42 * sizeof (int16_t);
void *ptr = malloc (n); /* { dg-message "" "note" } */
/* ^^^ on widening_svalues no expr is returned
by get_representative_tree at the moment. */
- int *iptr = (int *)ptr; /* { dg-line assign6 } */
+ int32_t *iptr = (int32_t *)ptr; /* { dg-line assign6 } */
free (iptr);
/* { dg-warning "allocated buffer size is not a multiple of the pointee's
size \\\[CWE-131\\\]" "warning" { target *-*-* } assign6 } */
- /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" {
target *-*-* } assign6 } */
+ /* { dg-message "'int32_t \\*' (\\\{aka 'int \\*'\\\})? here; &
[PATCH] analyzer: Fix handling of non-ints inside allocation size checker [PR106181]
This patch fixes the ICE reported in PR106181 by Arseny Solokha. With
this patch, the allocation size checker tries to handle floating-point
operands of allocation size arguments. Constant sizes get implicitly
converted and symbolic sizes are handled as long as the floating-point
operand could also be represented as a positive integer. In all other
cases and on unhandled constants, the checker falls back to not
emitting a warning.
Also, I unified the logic on zero byte allocations.
Regression-tested on x86_64 linux.
2022-07-05 Tim Lange
gcc/analyzer/ChangeLog:
PR analyzer/106181
* region-model.cc (capacity_compatible_with_type):
Can handle non-integer constants now.
(region_model::check_region_size): Adapted to the new signature of
capacity_compatible_with_type.
gcc/testsuite/ChangeLog:
PR analyzer/106181
* gcc.dg/analyzer/allocation-size-1.c: New tests.
* gcc.dg/analyzer/allocation-size-2.c: New tests.
* gcc.dg/analyzer/pr106181.c: New test.
---
gcc/analyzer/region-model.cc | 44 ---
.../gcc.dg/analyzer/allocation-size-1.c | 29 +++-
.../gcc.dg/analyzer/allocation-size-2.c | 22 ++
gcc/testsuite/gcc.dg/analyzer/pr106181.c | 7 +++
4 files changed, 95 insertions(+), 7 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr106181.c
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 5d939327e01..e097ecb3c07 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -2904,13 +2904,45 @@ private:
static bool
capacity_compatible_with_type (tree cst, tree pointee_size_tree,
- bool is_struct)
+ bool is_struct, bool floor_real)
{
- gcc_assert (TREE_CODE (cst) == INTEGER_CST);
gcc_assert (TREE_CODE (pointee_size_tree) == INTEGER_CST);
-
unsigned HOST_WIDE_INT pointee_size = TREE_INT_CST_LOW (pointee_size_tree);
- unsigned HOST_WIDE_INT alloc_size = TREE_INT_CST_LOW (cst);
+
+ unsigned HOST_WIDE_INT alloc_size;
+ switch (TREE_CODE (cst))
+{
+default:
+ /* Assume all unhandled operands are compatible. */
+ return true;
+case INTEGER_CST:
+ alloc_size = TREE_INT_CST_LOW (cst);
+ break;
+case REAL_CST:
+ {
+ const REAL_VALUE_TYPE *rv = TREE_REAL_CST_PTR (cst);
+ if (floor_real)
+ {
+ /* If the size is constant real at compile-time,
+ we can model the conversion. */
+ alloc_size = real_to_integer (rv);
+ }
+ else
+ {
+ /* On expressions where the value of one operator isn't
+ representable as an integer or is negative, we give up and
+ just assume that the programmer knows what they are doing. */
+ HOST_WIDE_INT i;
+ if (real_isneg (rv) || !real_isinteger (rv, &i))
+ return true;
+ alloc_size = i;
+ }
+ }
+ break;
+}
+
+ if (alloc_size == 0)
+return true;
if (is_struct)
return alloc_size >= pointee_size;
@@ -2920,7 +2952,7 @@ capacity_compatible_with_type (tree cst, tree
pointee_size_tree,
static bool
capacity_compatible_with_type (tree cst, tree pointee_size_tree)
{
- return capacity_compatible_with_type (cst, pointee_size_tree, false);
+ return capacity_compatible_with_type (cst, pointee_size_tree, false, false);
}
/* Checks whether SVAL could be a multiple of SIZE_CST.
@@ -3145,7 +3177,7 @@ region_model::check_region_size (const region *lhs_reg,
const svalue *rhs_sval,
= as_a (capacity);
tree cst_cap = cst_cap_sval->get_constant ();
if (!capacity_compatible_with_type (cst_cap, pointee_size_tree,
- is_struct))
+ is_struct, true))
ctxt->warn (new dubious_allocation_size (lhs_reg, rhs_reg,
cst_cap));
}
diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
index 4a78a81d054..1a1c8e75c98 100644
--- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
@@ -114,4 +114,31 @@ void test_10 (int32_t n)
{
char *ptr = malloc (7 * n);
free (ptr);
-}
\ No newline at end of file
+}
+
+void test_11 ()
+{
+ int32_t *ptr = malloc (3.0); /* { dg-line malloc11 } */
+ free (ptr);
+ /* { dg-warning "allocated buffer size is not a multiple of the pointee's
size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc11 } */
+ /* { dg-message "'int32_t \\*' (\\\{aka '(long )?int \\*'\\\})? here;
'sizeof \\(int32_t (\\\{aka (long )?int\\\})?\\)' is '4'" "note" { target *-*-*
} malloc11 } */
+}
+
+void test_12 ()
+
Re: Floating-point allocation sizes? (was Re: [PATCH] analyzer: Fix handling of non-ints inside allocation size checker [PR106181])
On Tue, Jul 5 2022 at 05:37:46 PM -0400, David Malcolm
wrote:
On Tue, 2022-07-05 at 21:49 +0200, Tim Lange wrote:
This patch fixes the ICE reported in PR106181 by Arseny Solokha.
With
this patch, the allocation size checker tries to handle
floating-point
operands of allocation size arguments. Constant sizes get implicitly
converted and symbolic sizes are handled as long as the
floating-point
operand could also be represented as a positive integer. In all
other
cases and on unhandled constants, the checker falls back to not
emitting a warning.
Also, I unified the logic on zero byte allocations.
Hi Tim
Thanks for the patch.
We definitely don't want to crash, but my "gut reaction" to the
testsuite examples was that we ought to be warning on them - using
floating point when calculating an allocation size seems like asking
for trouble.
In particular test_16's:
int32_t *ptr = malloc (n * 3.1);
feels to me like it deserves a warning. I suppose it could be valid
if
n is a multiple of 40 (so that the buffer is a multiple of 31 * 4 and
thus a multiple of 4), for small enough n that we don't lose
precision,
but that code seems very questionable - the comment says "just assume
that the programmer knows what they are doing", but I think anyone
using -fanalyzer is opting-in to have more fussy checking and would
probably want to be warned about such code.
While fixing that case, I thought what sane person would think of using
floating-point arithmetic here. The main reason why I chose to give up
here instead of complain was because the checker can't know the result
and it is strange enough such that it might be deliberately.
In that sense, we could also talk about allocating 0 bytes. What
happens there seems to be undefined behavior and
implementation-specific. I've again decided to say that allocating 0
bytes is strange enough that it must be deliberately. The same standard
you've linked also has a article about that case [0]. If all readers
can't thing of any use case, I can certainly rework that patch to warn
on such allocations.
- Tim
[0]
https://wiki.sei.cmu.edu/confluence/display/c/MEM04-C.+Beware+of+zero-length+allocations
I also wondered what happens on NAN, with e.g.
#include
void test_nan (void)
{
int *p = malloc (NAN * sizeof (int));
}
but we issue -Woverflow on that.
I'm thinking that perhaps we should have a new warning for floating
point buffer size calculations, though I'm not yet sure exactly how it
should work and how fussy it should be (e.g. complain about floating
point calculations vs complain about *any* floating point used as a
buffer size, etc).
Does anyone know of real world code that uses floating point in
buffer-
size calculations? (updating Subject accordingly) Is there code out
there that does this? It seems broken to me, but maybe there's a
valid
use-case that I can't think of.
The closest such rule I can think of is CERT-C's
"FLP02-C. Avoid using floating-point numbers when precise computation
is needed":
https://wiki.sei.cmu.edu/confluence/display/c/FLP02-C.+Avoid+using+floating-point+numbers+when+precise+computation+is+needed
Dave
Regression-tested on x86_64 linux.
2022-07-05 Tim Lange
gcc/analyzer/ChangeLog:
PR analyzer/106181
* region-model.cc (capacity_compatible_with_type):
Can handle non-integer constants now.
(region_model::check_region_size): Adapted to the new
signature
of
capacity_compatible_with_type.
gcc/testsuite/ChangeLog:
PR analyzer/106181
* gcc.dg/analyzer/allocation-size-1.c: New tests.
* gcc.dg/analyzer/allocation-size-2.c: New tests.
* gcc.dg/analyzer/pr106181.c: New test.
---
gcc/analyzer/region-model.cc | 44
---
.../gcc.dg/analyzer/allocation-size-1.c | 29 +++-
.../gcc.dg/analyzer/allocation-size-2.c | 22 ++
gcc/testsuite/gcc.dg/analyzer/pr106181.c | 7 +++
4 files changed, 95 insertions(+), 7 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr106181.c
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-
model.cc
index 5d939327e01..e097ecb3c07 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -2904,13 +2904,45 @@ private:
static bool
capacity_compatible_with_type (tree cst, tree pointee_size_tree,
- bool is_struct)
+ bool is_struct, bool floor_real)
{
- gcc_assert (TREE_CODE (cst) == INTEGER_CST);
gcc_assert (TREE_CODE (pointee_size_tree) == INTEGER_CST);
-
unsigned HOST_WIDE_INT pointee_size = TREE_INT_CST_LOW
(pointee_size_tree);
- unsigned HOST_WIDE_INT alloc_size = TREE_INT_CST_LOW (cst);
+
+ unsigned HOST_WIDE_INT alloc_size;
+ switch (TREE_CODE (cst))
+{
+default:
+
[PATCH] Fix handling of zero capacity regions in -Wanalyzer-allocation-size [PR106394]
This patch unifies the handling of zero capacity regions for structs
and other types in the allocation size checker.
Regression-tested on x86_64 Linux.
2022-07-22 Tim Lange
gcc/analyzer/ChangeLog:
PR analyzer/106394
* region-model.cc (capacity_compatible_with_type): Always return true
if alloc_size is zero.
gcc/testsuite/ChangeLog:
PR analyzer/106394
* gcc.dg/analyzer/pr106394.c: New test.
---
gcc/analyzer/region-model.cc | 2 +-
gcc/testsuite/gcc.dg/analyzer/pr106394.c | 19 +++
2 files changed, 20 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr106394.c
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 8b7b4e1f697..e01c30407c4 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -2956,7 +2956,7 @@ capacity_compatible_with_type (tree cst, tree
pointee_size_tree,
unsigned HOST_WIDE_INT alloc_size = TREE_INT_CST_LOW (cst);
if (is_struct)
-return alloc_size >= pointee_size;
+return alloc_size == 0 || alloc_size >= pointee_size;
return alloc_size % pointee_size == 0;
}
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr106394.c
b/gcc/testsuite/gcc.dg/analyzer/pr106394.c
new file mode 100644
index 000..96bb175fc14
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr106394.c
@@ -0,0 +1,19 @@
+struct msm_gpu {
+ // [...snip...]
+ const struct msm_gpu_perfcntr *perfcntrs;
+ // [...snip...]
+};
+
+struct msm_gpu_perfcntr {
+ // [...snip...]
+ const char *name;
+};
+
+static const struct msm_gpu_perfcntr perfcntrs[] = {};
+
+struct msm_gpu *test(struct msm_gpu *gpu) {
+ // [...snip...]
+ gpu->perfcntrs = perfcntrs;
+ // [...snip...]
+ return gpu;
+}
--
2.36.1
[PATCH 1/2] analyzer: consider that realloc could shrink the buffer [PR106539]
This patch adds the "shrinks buffer" case to the success_with_move
modelling of realloc.
2022-08-09 Tim Lange
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
[PATCH 2/2] analyzer: out-of-bounds checker [PR106000]
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 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
[PATCH 1/2 v2] analyzer: consider that realloc could shrink the buffer [PR106539]
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
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 000..fd270868e36
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr106539.c
@@ -0,0 +1,15 @@
+#include
+
+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 ''" "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 000..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
[PATCH 2/2 v2] analyzer: out-of-bounds checker [PR106000]
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 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
[committed] testsuite: Disable out-of-bounds checker in analyzer/torture/pr93451.c
This patch disables Wanalyzer-out-of-bounds for analyzer/torture/pr93451.c
and makes the test case pass when compiled with -m32.
The emitted warning is a true positive but only occurs if
sizeof (long int) is less than sizeof (double). I've already discussed a
similar case with Dave in the context of pr96764.c and we came to the
conclusion that we just disable the checker in such cases.
Committed under the "obvious fix" rule.
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/torture/pr93451.c:
Disable Wanalyzer-out-of-bounds.
---
gcc/testsuite/gcc.dg/analyzer/torture/pr93451.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/gcc/testsuite/gcc.dg/analyzer/torture/pr93451.c
b/gcc/testsuite/gcc.dg/analyzer/torture/pr93451.c
index 5908bc4b69f..daac745d504 100644
--- a/gcc/testsuite/gcc.dg/analyzer/torture/pr93451.c
+++ b/gcc/testsuite/gcc.dg/analyzer/torture/pr93451.c
@@ -1,3 +1,5 @@
+/* { dg-additional-options "-Wno-analyzer-out-of-bounds" } */
+
void
mt (double);
--
2.37.1
[PATCH] analyzer: warn on the use of floating points in the size argument [PR106181]
This patch fixes the ICE reported in PR106181 and adds a new warning to
the analyzer complaining about the use of floating point operands.
I decided to move the warning for floats inside the size argument out of
the allocation size checker code and toward the allocation such that the
warning only appears once.
I'm not sure about the wording of the diagnostic, my current wording feels
a bit bulky. Here is how the diagnostics look like:
/path/to/main.c: In function ‘test_1’:
/path/to/main.c:10:14: warning: use of floating point arithmetic inside the
size argument might yield unexpected results
[-Wanalyzer-imprecise-floating-point-arithmetic]
10 | int *ptr = malloc (sizeof (int) * n); /* { dg-line test_1 } */
| ^
‘test_1’: event 1
|
| 10 | int *ptr = malloc (sizeof (int) * n); /* { dg-line test_1 } */
| | ^
| | |
| | (1) operand ‘n’ is of type ‘float’
|
/path/to/main.c:10:14: note: only use operands of a type that represents whole
numbers inside the size argument
/path/to/main.c: In function ‘test_2’:
/path/to/main.c:20:14: warning: use of floating point arithmetic inside the
size argument might yield unexpected results
[-Wanalyzer-imprecise-floating-point-arithmetic]
20 | int *ptr = malloc (n * 3.1); /* { dg-line test_2 } */
| ^~~~
‘test_2’: event 1
|
| 20 | int *ptr = malloc (n * 3.1); /* { dg-line test_2 } */
| | ^~~~
| | |
| | (1) operand ‘3.1001e+0’ is of type
‘double’
|
/path/to/main.c:20:14: note: only use operands of a type that represents whole
numbers inside the size argument
Also, another point to discuss is the event note in case the expression is
wrapped in a variable, such as in test_3:
/path/to/main.c:30:10: warning: use of floating point arithmetic inside the
size argument might yield unexpected results
[-Wanalyzer-imprecise-floating-point-arithmetic]
30 | return malloc (size); /* { dg-line test_3 } */
| ^
‘test_3’: events 1-2
|
| 37 | void test_3 (float f)
| | ^~
| | |
| | (1) entry to ‘test_3’
| 38 | {
| 39 | void *ptr = alloc_me (f); /* { dg-message "calling 'alloc_me'
from 'test_3'" } */
| |
| | |
| | (2) calling ‘alloc_me’ from ‘test_3’
|
+--> ‘alloc_me’: events 3-4
|
| 28 | void *alloc_me (size_t size)
| | ^~~~
| | |
| | (3) entry to ‘alloc_me’
| 29 | {
| 30 | return malloc (size); /* { dg-line test_3 } */
| | ~
| | |
| | (4) operand ‘f’ is of type ‘float’
|
I'm not sure if it is easily discoverable that event (4) does refer to
'size'. I thought about also printing get_representative_tree (capacity)
in the event but that would clutter the event message if the argument
does hold the full expression. I don't have any strong feelings about the
decision here but if I had to decide I'd leave it as is (especially
because the warning is probably quite unusual).
The index of the argument would also be a possibility, but that would get
tricky for calloc.
Regrtested on Linux x86_64, ran the analyzer & analyzer-torture tests with
the -m32 option enabled and had no false positives on coreutils, httpd,
openssh and curl.
2022-08-15 Tim Lange
gcc/analyzer/ChangeLog:
PR analyzer/106181
* analyzer.opt: Add Wanalyzer-imprecise-floating-point-arithmetic.
* region-model-impl-calls.cc (region_model::impl_call_alloca):
Add call to region_model::check_region_capacity_for_floats.
(region_model::impl_call_calloc):
Add call to region_model::check_region_capacity_for_floats.
(region_model::impl_call_malloc):
Add call to region_model::check_region_capacity_for_floats.
* region-model.cc (is_any_cast_p): Formatting.
(region_model::check_region_size): Ensure precondition.
(class imprecise_floating_point_arithmetic): New abstract
diagnostic class for all floating point related warnings.
(class float_as_size_arg): Concrete diagnostic class to complain
about floating point operands inside the size argument.
(class contains_floating_point_visitor):
New visitor to find floating point operands inside svalues.
(region_model::check_region_capacity_for_floats):
New function.
* region-model.h (class region_model):
Add region_model::check_region_capacity_for_floats.
gcc/ChangeLog:
[PATCH v2] analyzer: warn on the use of floating-points operands in the size argument [PR106181]
Hi,
this is the revised version of my patch. I had trouble to get your
point regarding the float_visitor:
> If the constant is seen first, then the non-constant won't be favored
> (though perhaps binary ops get canonicalized so that constants are on
> the RHS?).
Only the assignment of m_result in visit_constant_svalue is guarded by
!m_result, while the other two are not. So, there are two possibilities:
1. A constant is seen first and then assigned to m_result.
1.1. A non-constant float operand is seen later and
overwrites m_result.
1.2. There's no non-constant float operand, thus the
constant is the actual floating-point operand and
is kept inside m_result.
2. A non-constant is seen first, then m_result might be
overwritten with another non-constant later but never
with a constant.
Do I have a flaw in my thinking? (But they do seem to get canonicalized,
so that shouldn't matter)
> How about:
> -Wanalyzer-imprecise-float-arithmetic
> -Wanalyzer-imprecise-fp-arithmetic
> instead? (ideas welcome)
I've chosen the second. I mostly tried to avoid float because it is also
a reserved keyword in many languages and I wanted to avoid confusion
(might be overthinking that).
- Tim
This patch fixes the ICE reported in PR106181 and adds a new warning to
the analyzer complaining about the use of floating-point operands.
Regrtested on Linux x86_64.
2022-08-17 Tim Lange
gcc/analyzer/ChangeLog:
PR analyzer/106181
* analyzer.opt: Add Wanalyzer-imprecise-floating-point-arithmetic.
* region-model.cc (is_any_cast_p): Formatting.
(region_model::check_region_size): Ensure precondition.
(class imprecise_floating_point_arithmetic): New abstract
diagnostic class for all floating-point related warnings.
(class float_as_size_arg): Concrete diagnostic class to complain
about floating-point operands inside the size argument.
(class contains_floating_point_visitor):
New visitor to find floating-point operands inside svalues.
(region_model::check_dynamic_size_for_floats): New function.
(region_model::set_dynamic_extents):
Call to check_dynamic_size_for_floats.
* region-model.h (class region_model):
Add region_model::check_dynamic_size_for_floats.
gcc/ChangeLog:
PR analyzer/106181
* doc/invoke.texi: Add Wanalyzer-imprecise-fp-arithmetic.
gcc/testsuite/ChangeLog:
PR analyzer/106181
* gcc.dg/analyzer/allocation-size-1.c: New test.
* gcc.dg/analyzer/imprecise-floating-point-1.c: New test.
* gcc.dg/analyzer/pr106181.c: New test.
---
gcc/analyzer/analyzer.opt | 4 +
gcc/analyzer/region-model.cc | 179 +++---
gcc/analyzer/region-model.h | 2 +
gcc/doc/invoke.texi | 14 ++
.../gcc.dg/analyzer/allocation-size-1.c | 10 +
.../analyzer/imprecise-floating-point-1.c | 74
gcc/testsuite/gcc.dg/analyzer/pr106181.c | 11 ++
7 files changed, 271 insertions(+), 23 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr106181.c
diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
index 61b58c575ff..437ea92e130 100644
--- a/gcc/analyzer/analyzer.opt
+++ b/gcc/analyzer/analyzer.opt
@@ -98,6 +98,10 @@ Wanalyzer-free-of-non-heap
Common Var(warn_analyzer_free_of_non_heap) Init(1) Warning
Warn about code paths in which a non-heap pointer is freed.
+Wanalyzer-imprecise-fp-arithmetic
+Common Var(warn_analyzer_imprecise_fp_arithmetic) Init(1) Warning
+Warn about code paths in which floating-point arithmetic is used in locations
where precise computation is needed.
+
Wanalyzer-jump-through-null
Common Var(warn_analyzer_jump_through_null) Init(1) Warning
Warn about code paths in which a NULL function pointer is called.
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index b5bc3efda32..ec29be259b5 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -3301,7 +3301,8 @@ public:
m.add_cwe (131);
return warning_meta (rich_loc, m, get_controlling_option (),
- "allocated buffer size is not a multiple of the pointee's size");
+"allocated buffer size is not a multiple"
+" of the pointee's size");
}
label_text
@@ -3396,21 +3397,20 @@ capacity_compatible_with_type (tree cst, tree
pointee_size_tree)
class size_visitor : public visitor
{
public:
- size_visitor (tree size_cst, const svalue *sval, constraint_manager *cm)
- : m_size_cst (size_cst), m_sval (sval), m_cm (cm)
+ size_visitor (tree size_cst, const s
[PATCH] analyzer: buffer overlap checker [PR105898]
Hi,
below is the patch for the buffer overlap checker in the analyzer. It
contains a working buffer overlap warning as well as most of the code
needed to also warn on the general case aka when arguments alias with an
argument passed to restrict-qualified parameter.
The current C standard draft states that aliases to restrict-qualified
parameters are defined behavior if neither the alias nor the
restrict-qualified parameter is written. We've not yet come to a
conclusion how to handle this and thus, the function doing the general
case check is not used. A possible call site has a TODO stating that and
the current limitations are documented in invoke.texi.
- Tim
This patch adds a new checker to complain about overlapping buffers on
calls to memcpy and mempcpy.
Regression-tested on Linux x86_64 and tested as usual on coreutils, curl,
httpd and openssh.
2022-08-21 Tim Lange
gcc/analyzer/ChangeLog:
PR analyzer/105898
* analyzer.opt: Add Wanalyzer-restrict.
* region-model-impl-calls.cc (region_model::impl_call_memcpy):
Add call to region_model::check_region_overlap.
(region_model::impl_call_mempcpy): New function.
* region-model.cc (class restrict_alias): Concrete diagnostic to
complain about the disregard of the restrict qualifier.
(class region_overlap): Concrete diagnostic to complain about
overlapping buffers.
(region_model::check_region_aliases): New function.
(region_model::check_region_overlap): New function.
(region_model::on_call_pre):
Add call to region_model::impl_call_mempcpy.
* region-model.h (class region_model):
Add check_region_aliases and check_region_overlap.
* region.cc (region::unwrap_cast): New helper function.
* region.h: Add region::unwrap_cast.
* svalue.cc (svalue::unwrap_cast): New helper function.
* svalue.h: Add svalue::unwrap_cast.
gcc/ChangeLog:
PR analyzer/105898
* doc/invoke.texi: Add Wanalyzer-restrict.
gcc/testsuite/ChangeLog:
PR analyzer/105898
* gcc.dg/analyzer/restrict-1.c: New test.
---
gcc/analyzer/analyzer.opt | 4 +
gcc/analyzer/region-model-impl-calls.cc| 25 +-
gcc/analyzer/region-model.cc | 284 ++
gcc/analyzer/region-model.h| 8 +
gcc/analyzer/region.cc | 11 +
gcc/analyzer/region.h | 1 +
gcc/analyzer/svalue.cc | 11 +
gcc/analyzer/svalue.h | 1 +
gcc/doc/invoke.texi| 17 +
gcc/testsuite/gcc.dg/analyzer/restrict-1.c | 413 +
10 files changed, 774 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/restrict-1.c
diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
index 437ea92e130..ae5ebdb0d41 100644
--- a/gcc/analyzer/analyzer.opt
+++ b/gcc/analyzer/analyzer.opt
@@ -142,6 +142,10 @@ Wanalyzer-putenv-of-auto-var
Common Var(warn_analyzer_putenv_of_auto_var) Init(1) Warning
Warn about code paths in which an on-stack buffer is passed to putenv.
+Wanalyzer-restrict
+Common Var(warn_analyzer_restrict) Init(1) Warning
+Warn about code paths in which an argument passed to a restrict-qualified
parameter aliases with another argument.
+
Wanalyzer-shift-count-negative
Common Var(warn_analyzer_shift_count_negative) Init(1) Warning
Warn about code paths in which a shift with negative count is attempted.
diff --git a/gcc/analyzer/region-model-impl-calls.cc
b/gcc/analyzer/region-model-impl-calls.cc
index 8eebd122d42..bc8e343643a 100644
--- a/gcc/analyzer/region-model-impl-calls.cc
+++ b/gcc/analyzer/region-model-impl-calls.cc
@@ -502,7 +502,6 @@ region_model::impl_call_malloc (const call_details &cd)
}
/* Handle the on_call_pre part of "memcpy" and "__builtin_memcpy". */
-// TODO: complain about overlapping src and dest.
void
region_model::impl_call_memcpy (const call_details &cd)
@@ -516,6 +515,9 @@ region_model::impl_call_memcpy (const call_details &cd)
const region *src_reg = deref_rvalue (src_ptr_sval, cd.get_arg_tree (1),
cd.get_ctxt ());
+ check_region_overlap (src_reg, /* src_idx */ 1, dest_reg, /* dst_idx */ 0,
+ num_bytes_sval, cd);
+
cd.maybe_set_lhs (dest_ptr_sval);
const region *sized_src_reg
@@ -527,6 +529,27 @@ region_model::impl_call_memcpy (const call_details &cd)
set_value (sized_dest_reg, src_contents_sval, cd.get_ctxt ());
}
+/* Handle the on_call_pre part of "mempcpy" and "__builtin_mempcpy". */
+
+void
+region_model::impl_call_mempcpy (const call_details &cd)
+{
+ const svalue *dest_ptr_sval = cd.get_arg_svalue (0);
+ const svalue *src_ptr_sval = cd.get_arg_svalue (1);
+ const svalue *num_bytes_sval = cd.get_arg_svalue (2);
+
+ const r
[PATCH 1/2] analyzer: return a concrete offset for cast_regions
This patch fixes a bug where maybe_fold_sub_svalue did not fold the
access of a single char from a string to a char when the offset was zero
because get_relative_concrete_offset did return false for cast_regions.
Regrtested on Linux x86_64.
2022-09-02 Tim Lange
gcc/analyzer/ChangeLog:
* region.cc (cast_region::get_relative_concrete_offset):
New overloaded method.
* region.h: Add cast_region::get_relative_concrete_offset.
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/fold-string-to-char.c: New test.
---
gcc/analyzer/region.cc | 10 ++
gcc/analyzer/region.h | 2 ++
gcc/testsuite/gcc.dg/analyzer/fold-string-to-char.c | 8
3 files changed, 20 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/fold-string-to-char.c
diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc
index f4aba6b9c88..9c8279b130d 100644
--- a/gcc/analyzer/region.cc
+++ b/gcc/analyzer/region.cc
@@ -1556,6 +1556,16 @@ cast_region::dump_to_pp (pretty_printer *pp, bool
simple) const
}
}
+/* Implementation of region::get_relative_concrete_offset vfunc
+ for cast_region. */
+
+bool
+cast_region::get_relative_concrete_offset (bit_offset_t *out) const
+{
+ *out = (int) 0;
+ return true;
+}
+
/* class heap_allocated_region : public region. */
/* Implementation of region::dump_to_pp vfunc for heap_allocated_region. */
diff --git a/gcc/analyzer/region.h b/gcc/analyzer/region.h
index d37584b7285..34ce1fa1714 100644
--- a/gcc/analyzer/region.h
+++ b/gcc/analyzer/region.h
@@ -1087,6 +1087,8 @@ public:
void accept (visitor *v) const final override;
void dump_to_pp (pretty_printer *pp, bool simple) const final override;
+ bool get_relative_concrete_offset (bit_offset_t *out) const final override;
+
const region *get_original_region () const { return m_original_region; }
private:
diff --git a/gcc/testsuite/gcc.dg/analyzer/fold-string-to-char.c
b/gcc/testsuite/gcc.dg/analyzer/fold-string-to-char.c
new file mode 100644
index 000..46139216bba
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fold-string-to-char.c
@@ -0,0 +1,8 @@
+#include "analyzer-decls.h"
+
+void test_1 (void)
+{
+ char str[] = "Hello";
+ char *ptr = str;
+ __analyzer_eval (ptr[0] == 'H'); /* { dg-warning "TRUE" } */
+}
--
2.37.2
[PATCH 2/2] analyzer: strcpy and strncpy semantics
Hi,
below is my patch for the strcpy and strncpy semantics inside the
analyzer, enabling the out-of-bounds checker to also complain about
overflows caused by those two functions.
As the plan is to reason about the inequality of symbolic values in the
future, I decided to use eval_condition to compare the number of bytes and
the string size for strncpy [0].
- Tim
[0] instead of only trying to handle cases where svalues are constant;
which was how I did it in an earlier draft discussed off-list.
This patch adds modelling for the semantics of strcpy and strncpy in the
simple case where the analyzer is able to reason about the inequality of
the size argument and the string size.
Regrtested on Linux x86_64.
2022-09-02 Tim Lange
gcc/analyzer/ChangeLog:
* region-model-impl-calls.cc (region_model::impl_call_strncpy):
New function.
* region-model.cc (region_model::on_call_pre):
Add call to impl_call_strncpy.
(region_model::get_string_size): New function.
* region-model.h (class region_model):
Add impl_call_strncpy and get_string_size.
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/out-of-bounds-4.c: New test.
* gcc.dg/analyzer/strcpy-3.c: New test.
* gcc.dg/analyzer/strncpy-1.c: New test.
---
gcc/analyzer/region-model-impl-calls.cc | 62 -
gcc/analyzer/region-model.cc | 33 +
gcc/analyzer/region-model.h | 4 +
.../gcc.dg/analyzer/out-of-bounds-4.c | 122 ++
gcc/testsuite/gcc.dg/analyzer/strcpy-3.c | 23
gcc/testsuite/gcc.dg/analyzer/strncpy-1.c | 23
6 files changed, 264 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-4.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/strcpy-3.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/strncpy-1.c
diff --git a/gcc/analyzer/region-model-impl-calls.cc
b/gcc/analyzer/region-model-impl-calls.cc
index 8eebd122d42..9f1ae020f4f 100644
--- a/gcc/analyzer/region-model-impl-calls.cc
+++ b/gcc/analyzer/region-model-impl-calls.cc
@@ -1019,13 +1019,69 @@ region_model::impl_call_strcpy (const call_details &cd)
const svalue *dest_sval = cd.get_arg_svalue (0);
const region *dest_reg = deref_rvalue (dest_sval, cd.get_arg_tree (0),
cd.get_ctxt ());
+ const svalue *src_sval = cd.get_arg_svalue (1);
+ const region *src_reg = deref_rvalue (src_sval, cd.get_arg_tree (1),
+ cd.get_ctxt ());
+ const svalue *src_contents_sval = get_store_value (src_reg,
+cd.get_ctxt ());
cd.maybe_set_lhs (dest_sval);
- check_region_for_write (dest_reg, cd.get_ctxt ());
+ /* Try to get the string size if SRC_REG is a string_region. */
+ const svalue *copied_bytes_sval = get_string_size (src_reg);
+ /* Otherwise, check if the contents of SRC_REG is a string. */
+ if (copied_bytes_sval->get_kind () == SK_UNKNOWN)
+copied_bytes_sval = get_string_size (src_contents_sval);
+
+ const region *sized_dest_reg
+= m_mgr->get_sized_region (dest_reg, NULL_TREE, copied_bytes_sval);
+ set_value (sized_dest_reg, src_contents_sval, cd.get_ctxt ());
+}
+
+/* Handle the on_call_pre part of "strncpy" and "__builtin_strncpy_chk". */
- /* For now, just mark region's contents as unknown. */
- mark_region_as_unknown (dest_reg, cd.get_uncertainty ());
+void
+region_model::impl_call_strncpy (const call_details &cd)
+{
+ const svalue *dest_sval = cd.get_arg_svalue (0);
+ const region *dest_reg = deref_rvalue (dest_sval, cd.get_arg_tree (0),
+cd.get_ctxt ());
+ const svalue *src_sval = cd.get_arg_svalue (1);
+ const region *src_reg = deref_rvalue (src_sval, cd.get_arg_tree (1),
+ cd.get_ctxt ());
+ const svalue *src_contents_sval = get_store_value (src_reg,
+cd.get_ctxt ());
+ const svalue *num_bytes_sval = cd.get_arg_svalue (2);
+
+ cd.maybe_set_lhs (dest_sval);
+
+ const svalue *string_size_sval = get_string_size (src_reg);
+ if (string_size_sval->get_kind () == SK_UNKNOWN)
+string_size_sval = get_string_size (src_contents_sval);
+
+ /* strncpy copies until a zero terminator is reached or n bytes were copied.
+ Determine the lesser of both here. */
+ tristate ts = eval_condition (string_size_sval, LT_EXPR, num_bytes_sval);
+ const svalue *copied_bytes_sval;
+ switch (ts.get_value ())
+{
+ case tristate::TS_TRUE:
+ copied_bytes_sval = string_size_sval;
+ break;
+ case tristate::TS_FALSE:
+ copied_bytes_sval = num_bytes_sval;
+ break;
+ case tristate::TS_UNKNOWN:
+ copied_bytes_sval
+ = m_mgr->get_or_create_unknown_svalue (size_type_node);
+ break;
+ default:
+
[PATCH 2/2 v2] analyzer: strcpy semantics
Hi Dave,
sorry about the strncpy thing, I should've been more careful. Below is the
patch with just the strcpy part.
- Tim
This patch adds modelling for the semantics of strcpy in the simple case
where the analyzer is able to infer a concrete string size.
Regrtested on Linux x86_64.
2022-09-04 Tim Lange
gcc/analyzer/ChangeLog:
* region-model-impl-calls.cc (region_model::impl_call_strcpy):
Handle the constant string case.
* region-model.cc (region_model::get_string_size):
New function to get the string size from a region or svalue.
* region-model.h (class region_model): Add get_string_size.
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/out-of-bounds-4.c: New test.
* gcc.dg/analyzer/strcpy-3.c: New test.
---
gcc/analyzer/region-model-impl-calls.cc | 16 -
gcc/analyzer/region-model.cc | 29 +
gcc/analyzer/region-model.h | 3 +
.../gcc.dg/analyzer/out-of-bounds-4.c | 65 +++
gcc/testsuite/gcc.dg/analyzer/strcpy-3.c | 23 +++
5 files changed, 133 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-4.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/strcpy-3.c
diff --git a/gcc/analyzer/region-model-impl-calls.cc
b/gcc/analyzer/region-model-impl-calls.cc
index 8eebd122d42..3790eaf2d79 100644
--- a/gcc/analyzer/region-model-impl-calls.cc
+++ b/gcc/analyzer/region-model-impl-calls.cc
@@ -1019,13 +1019,23 @@ region_model::impl_call_strcpy (const call_details &cd)
const svalue *dest_sval = cd.get_arg_svalue (0);
const region *dest_reg = deref_rvalue (dest_sval, cd.get_arg_tree (0),
cd.get_ctxt ());
+ const svalue *src_sval = cd.get_arg_svalue (1);
+ const region *src_reg = deref_rvalue (src_sval, cd.get_arg_tree (1),
+ cd.get_ctxt ());
+ const svalue *src_contents_sval = get_store_value (src_reg,
+cd.get_ctxt ());
cd.maybe_set_lhs (dest_sval);
- check_region_for_write (dest_reg, cd.get_ctxt ());
+ /* Try to get the string size if SRC_REG is a string_region. */
+ const svalue *copied_bytes_sval = get_string_size (src_reg);
+ /* Otherwise, check if the contents of SRC_REG is a string. */
+ if (copied_bytes_sval->get_kind () == SK_UNKNOWN)
+copied_bytes_sval = get_string_size (src_contents_sval);
- /* For now, just mark region's contents as unknown. */
- mark_region_as_unknown (dest_reg, cd.get_uncertainty ());
+ const region *sized_dest_reg
+= m_mgr->get_sized_region (dest_reg, NULL_TREE, copied_bytes_sval);
+ set_value (sized_dest_reg, src_contents_sval, cd.get_ctxt ());
}
/* Handle the on_call_pre part of "strlen". */
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index ec29be259b5..4ec18c86774 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -3218,6 +3218,35 @@ region_model::get_capacity (const region *reg) const
return m_mgr->get_or_create_unknown_svalue (sizetype);
}
+/* Return the string size, including the 0-terminator, if SVAL is a
+ constant_svalue holding a string. Otherwise, return an unknown_svalue. */
+
+const svalue *
+region_model::get_string_size (const svalue *sval) const
+{
+ tree cst = sval->maybe_get_constant ();
+ if (!cst || TREE_CODE (cst) != STRING_CST)
+return m_mgr->get_or_create_unknown_svalue (size_type_node);
+
+ tree out = build_int_cst (size_type_node, TREE_STRING_LENGTH (cst));
+ return m_mgr->get_or_create_constant_svalue (out);
+}
+
+/* Return the string size, including the 0-terminator, if REG is a
+ string_region. Otherwise, return an unknown_svalue. */
+
+const svalue *
+region_model::get_string_size (const region *reg) const
+{
+ const string_region *str_reg = dyn_cast (reg);
+ if (!str_reg)
+return m_mgr->get_or_create_unknown_svalue (size_type_node);
+
+ tree cst = str_reg->get_string_cst ();
+ tree out = build_int_cst (size_type_node, TREE_STRING_LENGTH (cst));
+ return m_mgr->get_or_create_constant_svalue (out);
+}
+
/* If CTXT is non-NULL, use it to warn about any problems accessing REG,
using DIR to determine if this access is a read or write. */
diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index 7ce832f6ce4..a1f2165e145 100644
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -793,6 +793,9 @@ class region_model
const svalue *get_capacity (const region *reg) const;
+ const svalue *get_string_size (const svalue *sval) const;
+ const svalue *get_string_size (const region *reg) const;
+
/* Implemented in sm-malloc.cc */
void on_realloc_with_move (const call_details &cd,
const svalue *old_ptr_sval,
diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-4.c
b/gcc/te
[PATCH][WIP?] analyzer: support for symbolic values in the out-of-bounds checker [PR106625]
Hi, below is my patch, adding support for reasoning about buffer overflows and overreads with symbolic offsets and capacities. I've already had one off-list feedback from Dave after sending him my preliminary work. Below, I'll be also answering some of the questions that came up during the first round. To reason about out-of-bounds with symbolic values, I have decided to do some simplifications: * Only reason in bytes, i.e. loosing some bits on bit-field accesses and bit ranges in the conversion. * Not trying to handle commutativeness [0]. * Require similar structure of the offset and the capacity svalue to complain about symbolic out-of-bounds. * A symbolic leaf operand [1] is positive if its type is unsigned. I do ignore that the operand could be zero. It wouldn't make much sense to have an offset that is always zero, but is not inferable statically such that the approxmiation here would yield a false-positive. In order to fully prevent the false-positive, we would have to give up many true positives. Dave also thinks that is a reasonable approximation. > Whats the slowdown of this patch? I found another optimization that tries to skip the structural equality check by trying referential equality (aka comparing pointers) first. That seems to do the trick and at least in my single run of compiling coreutils, curl, httpd and openssh with the current master and my patch enabled, I've found little to no overhead, at most ~5s CPU time [2]. > Why was the realloc implementation changed? With the patch, the analyzer can reason about simple inequalities of svalues. The previous way of getting the smaller of the current buffer size and the new buffer size was less accurate and lead to a false positive because it chose the wrong svalue. The change fixes that by using the same comparison function as the out-of-bounds checker. Also, I changed it to return the OLD_SIZE_SVAL by default, because I think I had a thinking error in my previous patch: Realloc implementations probably only move the buffer if the buffer grows. That means the old buffer is copied to the new one, only touching as many bytes as the old buffer had. The rest of the bytes is left uninitialized. I added [WIP?], because the regrtests are not yet finished but a similar version did pass them, so I assume thats okay to post it now for review and hand in the regrtests later [3]. - Tim [0] I have tried that earlier but it turned out to be too slow. [1] leaf == conjured, inital or constant svalue. [2] Note that I didn't run multiple tests and the compile farm is not isolated and I haven't done anything about caching etc. But at least the results show that there is no heavy slowdown. [3] The analyzer and analyzer-torture tests pass on my machine for C/C++. This patch adds support for reasoning about the inequality of two symbolic values in the special case specifically suited for reasoning about out-of-bounds past the end of the buffer. With this patch, the analyzer catches off-by-one errors and more even when the offset and capacity is symbolic. Tested on coreutils, curl, httpd and openssh. gcc/analyzer/ChangeLog: PR analyzer/106625 * analyzer.h (region_offset): Eliminate m_is_symbolic member. * region-model-impl-calls.cc (region_model::impl_call_realloc): Refine implementation to be more precise. * region-model.cc (class symbolic_past_the_end): Abstract diagnostic class to complain about accesses past the end with symbolic values. (class symbolic_buffer_overflow): Concrete diagnostic class to complain about buffer overflows with symbolic values. (class symbolic_buffer_overread): Concrete diagnostic class to complain about buffer overreads with symbolic values. (region_model::check_symbolic_bounds): New function. (maybe_get_integer_cst_tree): New helper function. (region_model::check_region_bounds): Add call to check_symbolic_bounds if offset is not concrete. (region_model::eval_condition_without_cm): Add support for EQ_EXPR and GT_EXPR with binaryop_svalues. (is_positive_svalue): New hleper function. (region_model::symbolic_greater_than): (region_model::structural_equality): New function to compare whether two svalues are structured the same, i.e. evaluate to the same value. (test_struct): Reflect changes to region::calc_offset. (test_var): Likewise. (test_array_2): Likewise and add selftest with symbolic i. * region-model.h (class region_model): Add check_symbolic_bounds, symbolic_greater_than and structural_equality. * region.cc (region::get_offset): Reflect changes to region::calc_offset. (region::calc_offset): Compute the symbolic offset if the offset is not concrete. (region::get_relative_symbolic_offset): New function to return the symbolic
[PATCH v2] analyzer: support for symbolic values in the out-of-bounds checker [PR106625]
Hi Dave, while re-reading that patch, I noticed a small mistake. I accidently did not move the op == PLUS_EXPR or MULT_EXPR guard in symbolic_greater_than when implementing the "eliminate operands on both sides" feature, which lead to the old patch also eliminating operands on both sides if the operator decreases the value, which is obviously wrong. I moved the guard outside and added test coverage for this in symbolic-gt-1.c. The patch passed the regrtests with the fix included. I assume it is still okay for trunk? - Tim This patch adds support for reasoning about the inequality of two symbolic values in the special case specifically suited for reasoning about out-of-bounds past the end of the buffer. With this patch, the analyzer catches off-by-one errors and more even when the offset and capacity is symbolic. Regrtested on Linux x86_64 and tested on coreutils, curl, httpd and openssh as usual. 2022-09-07 Tim Lange gcc/analyzer/ChangeLog: PR analyzer/106625 * analyzer.h (region_offset): Eliminate m_is_symbolic member. * region-model-impl-calls.cc (region_model::impl_call_realloc): Refine implementation to be more precise. * region-model.cc (class symbolic_past_the_end): Abstract diagnostic class to complain about accesses past the end with symbolic values. (class symbolic_buffer_overflow): Concrete diagnostic class to complain about buffer overflows with symbolic values. (class symbolic_buffer_overread): Concrete diagnostic class to complain about buffer overreads with symbolic values. (region_model::check_symbolic_bounds): New function. (maybe_get_integer_cst_tree): New helper function. (region_model::check_region_bounds): Add call to check_symbolic_bounds if offset is not concrete. (region_model::eval_condition_without_cm): Add support for EQ_EXPR and GT_EXPR with binaryop_svalues. (is_positive_svalue): New hleper function. (region_model::symbolic_greater_than): (region_model::structural_equality): New function to compare whether two svalues are structured the same, i.e. evaluate to the same value. (test_struct): Reflect changes to region::calc_offset. (test_var): Likewise. (test_array_2): Likewise and add selftest with symbolic i. * region-model.h (class region_model): Add check_symbolic_bounds, symbolic_greater_than and structural_equality. * region.cc (region::get_offset): Reflect changes to region::calc_offset. (region::calc_offset): Compute the symbolic offset if the offset is not concrete. (region::get_relative_symbolic_offset): New function to return the symbolic offset in bytes relative to its parent. (field_region::get_relative_symbolic_offset): Likewise. (element_region::get_relative_symbolic_offset): Likewise. (offset_region::get_relative_symbolic_offset): Likewise. (bit_range_region::get_relative_symbolic_offset): Likewise. * region.h: Add get_relative_symbolic_offset. * store.cc (binding_key::make): Reflect changes to region::calc_offset. (binding_map::apply_ctor_val_to_range): Likewise. (binding_map::apply_ctor_pair_to_child_region): Likewise. (binding_cluster::bind_compound_sval): Likewise. (binding_cluster::get_any_binding): Likewise. (binding_cluster::maybe_get_compound_binding): Likewise. gcc/ChangeLog: PR analyzer/106625 * doc/invoke.texi: State that the checker also reasons about symbolic values. gcc/testsuite/ChangeLog: PR analyzer/106625 * gcc.dg/analyzer/data-model-1.c: Change expected result. * gcc.dg/analyzer/out-of-bounds-5.c: New test. * gcc.dg/analyzer/out-of-bounds-realloc-grow.c: New test. * gcc.dg/analyzer/symbolic-gt-1.c: New test. --- gcc/analyzer/analyzer.h | 23 +- gcc/analyzer/region-model-impl-calls.cc | 39 +- gcc/analyzer/region-model.cc | 469 -- gcc/analyzer/region-model.h | 9 + gcc/analyzer/region.cc| 131 - gcc/analyzer/region.h | 17 +- gcc/analyzer/store.cc | 18 +- gcc/doc/invoke.texi | 8 +- gcc/testsuite/gcc.dg/analyzer/data-model-1.c | 3 +- .../gcc.dg/analyzer/out-of-bounds-5.c | 156 ++ .../analyzer/out-of-bounds-realloc-grow.c | 87 gcc/testsuite/gcc.dg/analyzer/symbolic-gt-1.c | 76 +++ 12 files changed, 941 insertions(+), 95 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-realloc-grow.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/symbolic-gt-1.c diff --git a/gcc/analyzer/an
[PATCH] analyzer: consider empty ranges and zero byte accesses [PR106845]
Hi,
see my patch below for a fix of pr106845. I decided to allow bit_ranges
and byte_ranges to have a size of zero and rather only add an assertion
to the functions that assume a non-zero size. That way is more elegant in
the caller than restricting byte_range to only represent non-empty ranges.
- Tim
This patch adds handling of empty ranges in bit_range and byte_range and
adds an assertion to member functions that assume a positive size.
Further, the patch fixes an ICE caused by an empty byte_range passed to
byte_range::exceeds_p.
Regression-tested on Linux x86_64.
2022-09-10 Tim Lange
gcc/analyzer/ChangeLog:
PR analyzer/106845
* region-model.cc (region_model::check_region_bounds):
Bail out if 0 bytes were accessed.
* store.cc (byte_range::dump_to_pp):
Add special case for empty ranges.
(byte_range::exceeds_p): Restrict to non-empty ranges.
(byte_range::falls_short_of_p): Restrict to non-empty ranges.
* store.h (bit_range::empty_p): New function.
(bit_range::get_last_byte_offset): Restrict to non-empty ranges.
(byte_range::empty_p): New function.
(byte_range::get_last_byte_offset): Restrict to non-empty ranges.
gcc/testsuite/ChangeLog:
PR analyzer/106845
* gcc.dg/analyzer/out-of-bounds-zero.c: New test.
* gcc.dg/analyzer/pr106845.c: New test.
---
gcc/analyzer/region-model.cc | 3 +
gcc/analyzer/store.cc | 12 +++-
gcc/analyzer/store.h | 12
.../gcc.dg/analyzer/out-of-bounds-zero.c | 67 +++
gcc/testsuite/gcc.dg/analyzer/pr106845.c | 11 +++
5 files changed, 103 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-zero.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr106845.c
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index d321e5b6479..82006405373 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -1826,6 +1826,9 @@ region_model::check_region_bounds (const region *reg,
/* Find out how many bytes were accessed. */
const svalue *num_bytes_sval = reg->get_byte_size_sval (m_mgr);
tree num_bytes_tree = maybe_get_integer_cst_tree (num_bytes_sval);
+ /* Bail out if 0 bytes are accessed. */
+ if (num_bytes_tree && zerop (num_bytes_tree))
+return;
/* Get the capacity of the buffer. */
const svalue *capacity = get_capacity (base_reg);
diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc
index ec5232cb055..1857d95f0b6 100644
--- a/gcc/analyzer/store.cc
+++ b/gcc/analyzer/store.cc
@@ -380,7 +380,11 @@ bit_range::as_byte_range (byte_range *out) const
void
byte_range::dump_to_pp (pretty_printer *pp) const
{
- if (m_size_in_bytes == 1)
+ if (m_size_in_bytes == 0)
+{
+ pp_string (pp, "empty");
+}
+ else if (m_size_in_bytes == 1)
{
pp_string (pp, "byte ");
pp_wide_int (pp, m_start_byte_offset, SIGNED);
@@ -455,7 +459,9 @@ 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 ())
+ gcc_assert (!empty_p ());
+
+ if (other.get_next_byte_offset () < get_next_byte_offset ())
{
/* THIS definitely exceeds OTHER. */
byte_offset_t start = MAX (get_start_byte_offset (),
@@ -477,6 +483,8 @@ bool
byte_range::falls_short_of_p (byte_offset_t offset,
byte_range *out_fall_short_bytes) const
{
+ gcc_assert (!empty_p ());
+
if (get_start_byte_offset () < offset)
{
/* THIS falls short of OFFSET. */
diff --git a/gcc/analyzer/store.h b/gcc/analyzer/store.h
index ac8b6853f4b..d172ee756c8 100644
--- a/gcc/analyzer/store.h
+++ b/gcc/analyzer/store.h
@@ -237,6 +237,11 @@ struct bit_range
void dump_to_pp (pretty_printer *pp) const;
void dump () const;
+ bool empty_p () const
+ {
+return m_size_in_bits == 0;
+ }
+
bit_offset_t get_start_bit_offset () const
{
return m_start_bit_offset;
@@ -247,6 +252,7 @@ struct bit_range
}
bit_offset_t get_last_bit_offset () const
{
+gcc_assert (!empty_p ());
return get_next_bit_offset () - 1;
}
@@ -297,6 +303,11 @@ struct byte_range
void dump_to_pp (pretty_printer *pp) const;
void dump () const;
+ bool empty_p () const
+ {
+return m_size_in_bytes == 0;
+ }
+
bool contains_p (byte_offset_t offset) const
{
return (offset >= get_start_byte_offset ()
@@ -329,6 +340,7 @@ struct byte_range
}
byte_offset_t get_last_byte_offset () const
{
+gcc_assert (!empty_p ());
return m_start_byte_offset + m_size_in_bytes - 1;
}
diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-zero.c
b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-zero.c
new file mode 100644
index 000..201ca00ebdb
--
Re: [PATCH] analyzer: consider empty ranges and zero byte accesses [PR106845]
> ...it took me a moment to realize that the analyzer "sees" that this is
> "main", and thus buf_size is 0.
>
> Interestingly, if I rename it to not be "main" (and thus buf_size could
> be non-zero), we still don't complain:
> https://godbolt.org/z/PezfTo9Mz
> Presumably this is a known limitation of the symbolic bounds checking?
Yeah. I do only try structural equality for binaryop_svalues. The
example does result in a call to eval_condition_without_cm with two
unaryop_svalue(NOP_EXPR, initial_svalue ('buf_size'))
that have different types ('unsigned int' and 'sizetype'). Thus,
lhs == rhs is false and eval_condition_without_cm does return UNKNOWN.
Changing the type of buf_size to size_t removes the UNARYOP wrapping and
thus, emits a warning: https://godbolt.org/z/4sh7TM4v1 [0]
Otherwise, we could also do a call to structural_equality for
unaryop_svalue inside eval_condition_without_cm and ignore a type
mismatch for unaryop_svalues. That way, the analyzer would complain about
your example. Not 100% sure but I think it is okay to ignore the type
here for unaryop_svalues as long as the leafs match up. If you agree,
I can prepare a patch [1].
[0] I've seen you pushed a patch that displays the capacity as a new
event at region_creation. My patches did that by overwriting whats
printed using describe_region_creation_event. Should I remove all
those now unneccessary describe_region_creation_event overloads?
[1] Below is how that would probably look like.
---
gcc/analyzer/region-model.cc | 22 +++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 82006405373..4a9f0ff1e86 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -4190,6 +4190,24 @@ region_model::eval_condition_without_cm (const svalue
*lhs,
}
}
+ if (lhs->get_kind () == SK_UNARYOP)
+{
+ switch (op)
+ {
+ default:
+ break;
+ case EQ_EXPR:
+ case LE_EXPR:
+ case GE_EXPR:
+ {
+ tristate res = structural_equality (lhs, rhs);
+ if (res.is_true ())
+ return res;
+ }
+ break;
+ }
+}
+
return tristate::TS_UNKNOWN;
}
@@ -4307,9 +4325,7 @@ region_model::structural_equality (const svalue *a, const
svalue *b) const
{
const unaryop_svalue *un_a = as_a (a);
if (const unaryop_svalue *un_b = dyn_cast (b))
- return tristate (pending_diagnostic::same_tree_p (un_a->get_type (),
- un_b->get_type ())
- && un_a->get_op () == un_b->get_op ()
+ return tristate (un_a->get_op () == un_b->get_op ()
&& structural_equality (un_a->get_arg (),
un_b->get_arg ()));
}
--
2.37.3
