From: Tim Lange <mail@tim-lange.me>
To: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
Cc: David Malcolm <dmalcolm@redhat.com>, GCC Mailing List <gcc@gcc.gnu.org>
Subject: Re: [RFC] analyzer: allocation size warning
Date: Fri, 17 Jun 2022 21:23:34 +0200 [thread overview]
Message-ID: <A7ZMDR.7J1GGPCTKYH3@tim-lange.me> (raw)
In-Reply-To: <CAAgBjMmQOLFBkveLGNxSqqLePOoDBG9-4gQouc8mWPpuoPhxEw@mail.gmail.com>
On Fr, Jun 17 2022 at 22:45:42 +0530, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
> On Fri, 17 Jun 2022 at 21:25, Tim Lange <mail@tim-lange.me> wrote:
>>
>> Hi everyone,
> Hi Tim,
> Thanks for posting the POC patch!
> Just a couple of comments (inline)
Hi Prathamesh,
thanks for looking at it.
>>
>> tracked in PR105900 [0], I'd like to add support for a new warning
>> on
>> dubious allocation sizes. The new checker emits a warning when the
>> allocation size is not a multiple of the type's size. With the
>> checker,
>> following mistakes are detected:
>> int *arr = malloc(3); // forgot to multiply by sizeof
>> arr[0] = ...;
>> arr[1] = ...;
>> or
>> int *buf = malloc (n + sizeof(int)); // probably should be *
>> instead
>> of +
>> Because it is implemented inside the analyzer, it also emits
>> warnings
>> when the buffer is first of type void* and later on casted to
>> something
>> else. Though, this also inherits a limitation. The checker can not
>> distinguish 2 * sizeof(short) from sizeof(int) because sizeof is
>> resolved and constants are folded at the point when the analyzer
>> runs.
>> As a mitigation, I plan to implement a check in the frontend that
>> emits
>> a warning if sizeof(lhs pointee type) is not part of the malloc
>> argument.
> IMHO, warning if sizeof(lhs pointee_type) is not present inside
> malloc, might not be a good idea because it
> would reject valid calls to malloc.
> For eg:
> (1)
> size_t size = sizeof(int);
> int *p = malloc (size);
>
> (2)
> void *p = malloc (sizeof(int));
> int *q = p;
Hm, that's right. Maybe only warn when there is a sizeof(type) in the
argument and the lhs pointee_type != type (except for void*, maybe
char* and "inherited" structs)?
>>
>> I'm looking for a first feedback on the phrasing of the diagnostics
>> as
>> well on the preliminary patch [1].
>>
>> On constant buffer sizes, the warnings looks like this:
>> warning: Allocated buffer size is not a multiple of the pointee's
>> size
>> [CWE-131] [-Wanalyzer-allocation-size]
>> 22 | int *ptr = malloc (10 + sizeof(int)); /* { dg-line malloc2
>> } */
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~
>> ‘test_2’: event 1
>> |
>> | 22 | int *ptr = malloc (10 + sizeof(int)); /* { dg-line
>> malloc2 }
>> */
>> | | ^~~~~~~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (1) Casting a 14 byte buffer to ‘int *’ leaves 2
>> trailing
>> bytes; either the allocated size is bogus or the type on the
>> left-hand
>> side is wrong
>> |
>>
>> On symbolic buffer sizes:
>> warning: Allocated buffer size is not a multiple of the pointee's
>> size
>> [CWE-131] [-Wanalyzer-allocation-size]
>> 33 | int *ptr = malloc (n + sizeof(int)); /* { dg-line malloc3 }
>> */
>> | ^~~~~~~~~~~~~~~~~~~~~~~~
>> ‘test_3’: event 1
>> |
>> | 33 | int *ptr = malloc (n + sizeof(int)); /* { dg-line
>> malloc3 }
>> */
>> | | ^~~~~~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (1) Allocation is incompatible with ‘int *’; either the
>> allocated size is bogus or the type on the left-hand side is wrong
>> |
> Won't the warning be incorrect if 'n' is a multiple of sizeof(int) ?
> I assume by symbolic buffer size, 'n' is not known at compile time.
* VLAs are resolved to n * sizeof(type) when the analyzer runs and work
fine.
* Flows with if (cond) n = ...; else n = ...; are tracked by the
analyzer with a widening_svalue and can be handled (While thinking
about this answer, I noticed my patch is missing this case. Thanks!)
* In case of more complicated flows, the analyzer's buffer size
tracking resorts to unknown_svalue. If any variable in an expression is
unknown, no warning will be emitted.
* Generally, when requesting memory for a variable type, accepting an
arbitrary number doesn't sound right. I do warn, e.g. if 'n' is a
conjured_svalue (e.g. a from scanf call).
I think only the last case could in theory be a false-positive. I've
noticed that this is the case when 'n' is guarded by an if making sure
n is only a multiple of sizeof(type). In theory, I can fix this case
too as the analysis is path-sensitive.
Do you know of some other case where 'n' might be an unknown value
neither guarded an if condition nor resorted to 'unknown' by a
complicated flow but still correct?
- Tim
>
> Thanks,
> Prathamesh
>>
>> And this is how a simple flow looks like:
>> warning: Allocated buffer size is not a multiple of the pointee's
>> size
>> [CWE-131] [-Wanalyzer-allocation-size]
>> 39 | int *iptr = (int *)ptr; /* { dg-line assign } */
>> | ^~~~
>> ‘test_4’: events 1-2
>> |
>> | 38 | void *ptr = malloc (n * sizeof (short)); /* { dg-message
>> } */
>> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (1) allocated here
>> | 39 | int *iptr = (int *)ptr; /* { dg-line assign } */
>> | | ~~~~
>> | | |
>> | | (2) ‘ptr’ is incompatible with ‘int *’; either the
>> allocated size at (1) is bogus or the type on the left-hand side is
>> wrong
>> |
>>
>> There are some things to discuss from my side:
>> * The tests with the "toy re-implementation of CPython's object
>> model"[2] fail due to a extra warning emitted. Because the analyzer
>> can't know the calculation actually results in a correct buffer size
>> when viewed as a string_obj later on, it emits a warning, e.g. at
>> line
>> 61 in data-model-5.c. The only mitigation would be to disable the
>> warning for structs entirely. Now, the question is to rather have
>> noise
>> on these cases or disable the warning for structs entirely?
>> * I'm unable to emit a warning whenever the cast happens at an
>> assignment with a call as the rhs, e.g. test_1 in
>> allocation-size-4.c.
>> This is because I'm unable to access a region_svalue for the
>> returned
>> value. Even in the new_program_state, the svalue of the lhs is
>> still a
>> conjured_svalue. Maybe David can lead me to a place where I can
>> access
>> the return value's region_svalue or do I have to adapt the engine?
>> * attr-malloc-6.c and pr96639.c did both contain structs without an
>> implementation. Something in the analyzer must have triggered
>> another
>> warning about the usage of those without them having an
>> implementation.
>> I changed those structs to have an empty implementation, such that
>> the
>> additional warning are gone. I think this shouldn't change the test
>> case, so is this change okay?
>>
>> - Tim
>>
>> [0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105900
>> [1] While all tests except the cpython ones work, I have yet to
>> test it
>> on large C projects
>> [2] FAIL: gcc.dg/analyzer/data-model-5.c (test for excess errors)
>> FAIL: gcc.dg/analyzer/data-model-5b.c (test for excess errors)
>> FAIL: gcc.dg/analyzer/data-model-5c.c (test for excess errors)
>> FAIL: gcc.dg/analyzer/data-model-5d.c (test for excess errors)
>> FAIL: gcc.dg/analyzer/first-field-2.c (test for excess errors)
>>
>> -------
>>
>> Subject: [PATCH] analyzer: add allocation size warning
>>
>> This patch adds an allocation size checker to the analyzer.
>> The checker warns when the tracked buffer size is not a multiple of
>> the
>> left-hand side pointee's type. This resolves PR analyzer/105900.
>>
>> The patch is not yet fully tested.
>>
>> gcc/analyzer/ChangeLog:
>>
>> * analyzer.opt: Add Wanalyzer-allocation-size.
>> * sm-malloc.cc (class dubious_allocation_size): New
>> pending_diagnostic subclass.
>> (capacity_compatible_with_type): New.
>> (const_operand_in_sval_p): New.
>> (struct_or_union_with_inheritance_p): New.
>> (check_capacity): New.
>> (malloc_state_machine::on_stmt): Add calls to
>> on_pointer_assignment.
>> (malloc_state_machine::on_allocator_call): Add node to
>> parameters and call to on_pointer_assignment.
>> (malloc_state_machine::on_pointer_assignment): New.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * gcc.dg/analyzer/attr-malloc-6.c: Disabled
>> Wanalyzer-allocation-size and added default implementation for FILE.
>> * gcc.dg/analyzer/capacity-1.c: Added dg directives.
>> * gcc.dg/analyzer/malloc-4.c: Disabled
>> Wanalyzer-allocation-size.
>> * gcc.dg/analyzer/pr96639.c: Disabled
>> Wanalyzer-allocation-size
>> and added default implementation for foo and bar.
>> * gcc.dg/analyzer/allocation-size-1.c: New test.
>> * gcc.dg/analyzer/allocation-size-2.c: New test.
>> * gcc.dg/analyzer/allocation-size-3.c: New test.
>> * gcc.dg/analyzer/allocation-size-4.c: New test.
>>
>> Signed-off-by: Tim Lange <mail@tim-lange.me>
>> ---
>> gcc/analyzer/analyzer.opt | 4 +
>> gcc/analyzer/sm-malloc.cc | 363 +++++++++++++++++-
>> .../gcc.dg/analyzer/allocation-size-1.c | 54 +++
>> .../gcc.dg/analyzer/allocation-size-2.c | 44 +++
>> .../gcc.dg/analyzer/allocation-size-3.c | 48 +++
>> .../gcc.dg/analyzer/allocation-size-4.c | 39 ++
>> gcc/testsuite/gcc.dg/analyzer/attr-malloc-6.c | 2 +
>> gcc/testsuite/gcc.dg/analyzer/capacity-1.c | 5 +-
>> gcc/testsuite/gcc.dg/analyzer/malloc-4.c | 6 +-
>> gcc/testsuite/gcc.dg/analyzer/pr96639.c | 2 +
>> 10 files changed, 559 insertions(+), 8 deletions(-)
>> create mode 100644
>> gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
>> create mode 100644
>> gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
>> create mode 100644
>> gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c
>> create mode 100644
>> gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c
>>
>> diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
>> index 4aea52d3a87..f213989e0bb 100644
>> --- a/gcc/analyzer/analyzer.opt
>> +++ b/gcc/analyzer/analyzer.opt
>> @@ -78,6 +78,10 @@ Wanalyzer-malloc-leak
>> Common Var(warn_analyzer_malloc_leak) Init(1) Warning
>> Warn about code paths in which a heap-allocated pointer leaks.
>>
>> +Wanalyzer-allocation-size
>> +Common Var(warn_analyzer_allocation_size) Init(1) Warning
>> +Warn about code paths in which a buffer is assigned to a
>> incompatible
>> type.
>> +
>> Wanalyzer-mismatching-deallocation
>> Common Var(warn_analyzer_mismatching_deallocation) Init(1) Warning
>> Warn about code paths in which the wrong deallocation function is
>> called.
>> diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
>> index 3bd40425919..790c9f0e57d 100644
>> --- a/gcc/analyzer/sm-malloc.cc
>> +++ b/gcc/analyzer/sm-malloc.cc
>> @@ -46,6 +46,8 @@ along with GCC; see the file COPYING3. If not see
>> #include "attribs.h"
>> #include "analyzer/function-set.h"
>> #include "analyzer/program-state.h"
>> +#include "print-tree.h"
>> +#include "gimple-pretty-print.h"
>>
>> #if ENABLE_ANALYZER
>>
>> @@ -428,6 +430,7 @@ private:
>> get_or_create_deallocator (tree deallocator_fndecl);
>>
>> void on_allocator_call (sm_context *sm_ctxt,
>> + const supernode *node,
>> const gcall *call,
>> const deallocator_set *deallocators,
>> bool returns_nonnull = false) const;
>> @@ -444,6 +447,16 @@ private:
>> void on_realloc_call (sm_context *sm_ctxt,
>> const supernode *node,
>> const gcall *call) const;
>> + void on_pointer_assignment(sm_context *sm_ctxt,
>> + const supernode *node,
>> + const gassign *assign_stmt,
>> + tree lhs,
>> + tree rhs) const;
>> + void on_pointer_assignment(sm_context *sm_ctxt,
>> + const supernode *node,
>> + const gcall *call,
>> + tree lhs,
>> + tree rhs) const;
>> void on_zero_assignment (sm_context *sm_ctxt,
>> const gimple *stmt,
>> tree lhs) const;
>> @@ -1432,6 +1445,117 @@ private:
>> const char *m_funcname;
>> };
>>
>> +/* Concrete subclass for casts of pointers that lead to trailing
>> bytes. */
>> +
>> +class dubious_allocation_size : public malloc_diagnostic
>> +{
>> +public:
>> + dubious_allocation_size (const malloc_state_machine &sm, tree lhs,
>> tree rhs,
>> + tree size_tree, unsigned HOST_WIDE_INT size_diff)
>> + : malloc_diagnostic(sm, rhs),
>> m_type(dubious_allocation_type::CONSTANT_SIZE),
>> + m_lhs(lhs), m_size_tree(size_tree), m_size_diff(size_diff)
>> + {}
>> +
>> + dubious_allocation_size (const malloc_state_machine &sm, tree lhs,
>> tree rhs,
>> + tree size_tree)
>> + : malloc_diagnostic(sm, rhs),
>> m_type(dubious_allocation_type::MISSING_OPERAND),
>> + m_lhs(lhs), m_size_tree(size_tree), m_size_diff(0)
>> + {}
>> +
>> + const char *get_kind () const final override
>> + {
>> + return "dubious_allocation_size";
>> + }
>> +
>> + int get_controlling_option () const final override
>> + {
>> + return OPT_Wanalyzer_allocation_size;
>> + }
>> +
>> + bool subclass_equal_p (const pending_diagnostic &base_other) const
>> + final override
>> + {
>> + const dubious_allocation_size &other = (const
>> dubious_allocation_size
>> &)base_other;
>> + return malloc_diagnostic::subclass_equal_p(other)
>> + && m_type == other.m_type
>> + && same_tree_p (m_lhs, other.m_lhs)
>> + && same_tree_p (m_size_tree, other.m_size_tree)
>> + && m_size_diff == other.m_size_diff;
>> + }
>> +
>> + bool emit (rich_location *rich_loc) final override
>> + {
>> + diagnostic_metadata m;
>> + 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");
>> + }
>> +
>> + label_text describe_state_change (const evdesc::state_change
>> &change)
>> + override
>> + {
>> + if (change.m_old_state == m_sm.get_start_state ()
>> + && unchecked_p (change.m_new_state))
>> + {
>> + m_alloc_event = change.m_event_id;
>> + if (m_type == dubious_allocation_type::CONSTANT_SIZE)
>> + {
>> + // TODO: verify that it's the allocation stmt, not a copy
>> + return change.formatted_print ("%E bytes allocated here",
>> + m_size_tree);
>> + }
>> + }
>> + return malloc_diagnostic::describe_state_change (change);
>> + }
>> +
>> + label_text describe_final_event (const evdesc::final_event &ev)
>> final
>> override
>> + {
>> + if (m_type == dubious_allocation_type::CONSTANT_SIZE)
>> + {
>> + if (m_alloc_event.known_p ())
>> + return ev.formatted_print (
>> + "Casting %qE to %qT leaves %wu trailing bytes; either the"
>> + " allocated size is bogus or the type on the left-hand side is"
>> + " wrong",
>> + m_arg, TREE_TYPE (m_lhs), m_size_diff);
>> + else
>> + return ev.formatted_print (
>> + "Casting a %E byte buffer to %qT leaves %wu trailing bytes;
>> either"
>> + " the allocated size is bogus or the type on the left-hand side
>> is"
>> + " wrong",
>> + m_size_tree, TREE_TYPE (m_lhs), m_size_diff);
>> + }
>> + else if (m_type == dubious_allocation_type::MISSING_OPERAND)
>> + {
>> + if (m_alloc_event.known_p ())
>> + return ev.formatted_print (
>> + "%qE is incompatible with %qT; either the allocated size at %@ is"
>> + " bogus or the type on the left-hand side is wrong",
>> + m_arg, TREE_TYPE (m_lhs), &m_alloc_event);
>> + else
>> + return ev.formatted_print (
>> + "Allocation is incompatible with %qT; either the allocated size
>> is"
>> + " bogus or the type on the left-hand side is wrong",
>> + TREE_TYPE (m_lhs));
>> + }
>> +
>> + gcc_unreachable ();
>> + return label_text ();
>> + }
>> +
>> +private:
>> + enum dubious_allocation_type {
>> + CONSTANT_SIZE,
>> + MISSING_OPERAND
>> + };
>> +
>> + dubious_allocation_type m_type;
>> + diagnostic_event_id_t m_alloc_event;
>> + tree m_lhs;
>> + tree m_size_tree;
>> + unsigned HOST_WIDE_INT m_size_diff;
>> +};
>> +
>> /* struct allocation_state : public state_machine::state. */
>>
>> /* Implementation of state_machine::state::dump_to_pp vfunc
>> @@ -1633,6 +1757,160 @@ known_allocator_p (const_tree fndecl, const
>> gcall *call)
>> return false;
>> }
>>
>> +/* Returns the trailing bytes on dubious allocation sizes. */
>> +
>> +static unsigned HOST_WIDE_INT
>> +capacity_compatible_with_type (tree cst, tree pointee_size_tree)
>> +{
>> + unsigned HOST_WIDE_INT pointee_size = TREE_INT_CST_LOW
>> (pointee_size_tree);
>> + if (pointee_size == 0)
>> + return 0;
>> + unsigned HOST_WIDE_INT alloc_size = TREE_INT_CST_LOW (cst);
>> +
>> + return alloc_size % pointee_size;
>> +}
>> +
>> +/* Returns true if there is a constant tree with
>> + the same constant value inside the sval. */
>> +
>> +static bool
>> +const_operand_in_sval_p (const svalue *sval, tree size_cst)
>> +{
>> + auto_vec<const svalue *> non_mult_expr;
>> + auto_vec<const svalue *> worklist;
>> + worklist.safe_push(sval);
>> + while (!worklist.is_empty())
>> + {
>> + const svalue *curr = worklist.pop ();
>> + curr = curr->unwrap_any_unmergeable ();
>> +
>> + switch (curr->get_kind())
>> + {
>> + default:
>> + break;
>> + case svalue_kind::SK_CONSTANT:
>> + {
>> + const constant_svalue *cst_sval = curr->dyn_cast_constant_svalue
>> ();
>> + unsigned HOST_WIDE_INT sval_int
>> + = TREE_INT_CST_LOW (cst_sval->get_constant ());
>> + unsigned HOST_WIDE_INT size_cst_int = TREE_INT_CST_LOW (size_cst);
>> + if (sval_int % size_cst_int == 0)
>> + return true;
>> + }
>> + break;
>> + case svalue_kind::SK_BINOP:
>> + {
>> + const binop_svalue *b_sval = curr->dyn_cast_binop_svalue ();
>> + if (b_sval->get_op () == MULT_EXPR)
>> + {
>> + worklist.safe_push (b_sval->get_arg0 ());
>> + worklist.safe_push (b_sval->get_arg1 ());
>> + }
>> + else
>> + {
>> + non_mult_expr.safe_push (b_sval->get_arg0 ());
>> + non_mult_expr.safe_push (b_sval->get_arg1 ());
>> + }
>> + }
>> + break;
>> + case svalue_kind::SK_UNARYOP:
>> + {
>> + const unaryop_svalue *un_sval = curr->dyn_cast_unaryop_svalue ();
>> + worklist.safe_push (un_sval->get_arg ());
>> + }
>> + break;
>> + case svalue_kind::SK_UNKNOWN:
>> + return true;
>> + }
>> + }
>> +
>> + /* Each expr should be a multiple of the size.
>> + E.g. used to catch n + sizeof(int) errors. */
>> + bool reduce = !non_mult_expr.is_empty ();
>> + while (!non_mult_expr.is_empty() && reduce)
>> + {
>> + const svalue *expr_sval = non_mult_expr.pop ();
>> + reduce &= const_operand_in_sval_p (expr_sval, size_cst);
>> + }
>> + return reduce;
>> +}
>> +
>> +/* Returns true iff the type is a struct with another struct
>> inside. */
>> +
>> +static bool
>> +struct_or_union_with_inheritance_p (tree type)
>> +{
>> + if (!RECORD_OR_UNION_TYPE_P (type))
>> + return false;
>> +
>> + for (tree f = TYPE_FIELDS (type); f; f = TREE_CHAIN (f))
>> + if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (f)))
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +static void
>> +check_capacity (sm_context *sm_ctxt,
>> + const malloc_state_machine &sm,
>> + const supernode *node,
>> + const gimple *stmt,
>> + tree lhs,
>> + tree rhs,
>> + const svalue *capacity)
>> +{
>> + tree pointer_type = TREE_TYPE (lhs);
>> + gcc_assert (TREE_CODE (pointer_type) == POINTER_TYPE);
>> +
>> + tree pointee_type = TREE_TYPE (pointer_type);
>> + /* void * is always compatible. */
>> + if (TREE_CODE (pointee_type) == VOID_TYPE)
>> + return;
>> +
>> + if (struct_or_union_with_inheritance_p (pointee_type))
>> + return;
>> +
>> + tree pointee_size_tree = size_in_bytes(pointee_type);
>> + /* The size might be unknown e.g. being a array with n elements
>> + or casting to char * never has any trailing bytes. */
>> + if (TREE_CODE (pointee_size_tree) != INTEGER_CST
>> + || TREE_INT_CST_LOW (pointee_size_tree) == 1)
>> + return;
>> +
>> + switch (capacity->get_kind ())
>> + {
>> + default:
>> + break;
>> + case svalue_kind::SK_CONSTANT:
>> + {
>> + const constant_svalue *cst_sval =
>> capacity->dyn_cast_constant_svalue
>> ();
>> + tree cst = cst_sval->get_constant ();
>> + unsigned HOST_WIDE_INT size_diff
>> + = capacity_compatible_with_type (cst, pointee_size_tree);
>> + if (size_diff != 0)
>> + {
>> + tree diag_arg = sm_ctxt->get_diagnostic_tree (rhs);
>> + sm_ctxt->warn (node, stmt, diag_arg,
>> + new dubious_allocation_size (sm, lhs, diag_arg,
>> + cst, size_diff));
>> + }
>> + }
>> + break;
>> + case svalue_kind::SK_BINOP:
>> + case svalue_kind::SK_UNARYOP:
>> + {
>> + if (!const_operand_in_sval_p (capacity, pointee_size_tree))
>> + {
>> + tree diag_arg = sm_ctxt->get_diagnostic_tree (rhs);
>> + sm_ctxt->warn (node, stmt, diag_arg,
>> + new dubious_allocation_size (sm, lhs, diag_arg,
>> + pointee_size_tree));
>> + }
>> + }
>> + break;
>> + }
>> +}
>> +
>> /* Implementation of state_machine::on_stmt vfunc for
>> malloc_state_machine. */
>>
>> bool
>> @@ -1645,14 +1923,14 @@ malloc_state_machine::on_stmt (sm_context
>> *sm_ctxt,
>> {
>> if (known_allocator_p (callee_fndecl, call))
>> {
>> - on_allocator_call (sm_ctxt, call, &m_free);
>> + on_allocator_call (sm_ctxt, node, call, &m_free);
>> return true;
>> }
>>
>> if (is_named_call_p (callee_fndecl, "operator new", call, 1))
>> - on_allocator_call (sm_ctxt, call, &m_scalar_delete);
>> + on_allocator_call (sm_ctxt, node, call, &m_scalar_delete);
>> else if (is_named_call_p (callee_fndecl, "operator new []", call,
>> 1))
>> - on_allocator_call (sm_ctxt, call, &m_vector_delete);
>> + on_allocator_call (sm_ctxt, node, call, &m_vector_delete);
>> else if (is_named_call_p (callee_fndecl, "operator delete", call,
>> 1)
>> || is_named_call_p (callee_fndecl, "operator delete", call, 2))
>> {
>> @@ -1707,7 +1985,7 @@ malloc_state_machine::on_stmt (sm_context
>> *sm_ctxt,
>> tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (callee_fndecl));
>> bool returns_nonnull
>> = lookup_attribute ("returns_nonnull", attrs);
>> - on_allocator_call (sm_ctxt, call, deallocators, returns_nonnull);
>> + on_allocator_call (sm_ctxt, node, call, deallocators,
>> returns_nonnull);
>> }
>>
>> /* Handle "__attribute__((nonnull))". */
>> @@ -1763,12 +2041,31 @@ malloc_state_machine::on_stmt (sm_context
>> *sm_ctxt,
>> = mutable_this->get_or_create_deallocator (callee_fndecl);
>> on_deallocator_call (sm_ctxt, node, call, d, dealloc_argno);
>> }
>> +
>> + /* Handle returns from function calls. */
>> + tree lhs = gimple_call_lhs (call);
>> + if (lhs && TREE_CODE (TREE_TYPE (lhs)) == POINTER_TYPE
>> + && TREE_CODE (gimple_call_return_type (call)) == POINTER_TYPE)
>> + on_pointer_assignment (sm_ctxt, node, call, lhs,
>> + gimple_call_fn (call));
>> }
>>
>> if (tree lhs = sm_ctxt->is_zero_assignment (stmt))
>> if (any_pointer_p (lhs))
>> on_zero_assignment (sm_ctxt, stmt,lhs);
>>
>> + /* Handle pointer assignments/casts for dubious allocation size.
>> */
>> + if (const gassign *assign_stmt = dyn_cast <const gassign *>
>> (stmt))
>> + {
>> + if (gimple_num_ops (stmt) == 2)
>> + {
>> + tree lhs = gimple_assign_lhs (assign_stmt);
>> + tree rhs = gimple_assign_rhs1 (assign_stmt);
>> + if (any_pointer_p (lhs) && any_pointer_p (rhs))
>> + on_pointer_assignment (sm_ctxt, node, assign_stmt, lhs, rhs);
>> + }
>> + }
>> +
>> /* Handle dereferences. */
>> for (unsigned i = 0; i < gimple_num_ops (stmt); i++)
>> {
>> @@ -1818,6 +2115,7 @@ malloc_state_machine::on_stmt (sm_context
>> *sm_ctxt,
>>
>> void
>> malloc_state_machine::on_allocator_call (sm_context *sm_ctxt,
>> + const supernode *node,
>> const gcall *call,
>> const deallocator_set *deallocators,
>> bool returns_nonnull) const
>> @@ -1830,6 +2128,9 @@ malloc_state_machine::on_allocator_call
>> (sm_context *sm_ctxt,
>> (returns_nonnull
>> ? deallocators->m_nonnull
>> : deallocators->m_unchecked));
>> +
>> + if (TREE_CODE (TREE_TYPE (lhs)) == POINTER_TYPE)
>> + on_pointer_assignment (sm_ctxt, node, call, lhs, gimple_call_fn
>> (call));
>> }
>> else
>> {
>> @@ -1968,6 +2269,60 @@ malloc_state_machine::on_realloc_call
>> (sm_context *sm_ctxt,
>> }
>> }
>>
>> +/* Handle assignments between two pointers.
>> + Check for dubious allocation sizes.
>> +*/
>> +
>> +void
>> +malloc_state_machine::on_pointer_assignment (sm_context *sm_ctxt,
>> + const supernode *node,
>> + const gassign *assign_stmt,
>> + tree lhs,
>> + tree rhs) const
>> +{
>> + /* Do not warn if lhs and rhs are of the same type to not emit
>> duplicate
>> + warnings on assignments after the cast. */
>> + if (pending_diagnostic::same_tree_p (TREE_TYPE (lhs), TREE_TYPE
>> (rhs)))
>> + return;
>> +
>> + const program_state *state = sm_ctxt->get_old_program_state ();
>> + const svalue *r_value = state->m_region_model->get_rvalue (rhs,
>> NULL);
>> + if (const region_svalue *reg = dyn_cast <const region_svalue *>
>> (r_value))
>> + {
>> + const svalue *capacity = state->m_region_model->get_capacity
>> + (reg->get_pointee ());
>> + check_capacity(sm_ctxt, *this, node, assign_stmt, lhs, rhs,
>> capacity);
>> + }
>> +}
>> +
>> +void
>> +malloc_state_machine::on_pointer_assignment (sm_context *sm_ctxt,
>> + const supernode *node,
>> + const gcall *call,
>> + tree lhs,
>> + tree fn_decl) const
>> +{
>> + /* Do not warn if lhs and rhs are of the same type to not emit
>> duplicate
>> + warnings on assignments after the cast. */
>> + if (pending_diagnostic::same_tree_p
>> + (TREE_TYPE (lhs), TREE_TYPE (gimple_call_return_type (call))))
>> + return;
>> +
>> + const program_state *state = sm_ctxt->get_new_program_state ();
>> + const svalue *r_value = state->m_region_model->get_rvalue (lhs,
>> NULL);
>> + if (const region_svalue *reg = dyn_cast <const region_svalue *>
>> (r_value))
>> + {
>> + const svalue *capacity = state->m_region_model->get_capacity
>> + (reg->get_pointee ());
>> + check_capacity (sm_ctxt, *this, node, call, lhs, fn_decl,
>> capacity);
>> + }
>> + else if (const conjured_svalue *con
>> + = dyn_cast <const conjured_svalue *> (r_value))
>> + {
>> + // FIXME: How to get a region_svalue?
>> + }
>> +}
>> +
>> /* Implementation of state_machine::on_phi vfunc for
>> malloc_state_machine. */
>>
>> void
>> diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
>> b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
>> new file mode 100644
>> index 00000000000..5403c5f41f1
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
>> @@ -0,0 +1,54 @@
>> +#include <stdlib.h>
>> +
>> +/* Tests with constant buffer sizes */
>> +
>> +void test_1 (void)
>> +{
>> + short *ptr = malloc (21 * sizeof(short));
>> + free (ptr);
>> +}
>> +
>> +void test_2 (void)
>> +{
>> + int *ptr = malloc (21 * sizeof (short)); /* { dg-line malloc } */
>> + free (ptr);
>> +
>> + /* { dg-warning "Allocated buffer size is not a multiple of the
>> pointee's size" "" { target *-*-* } malloc } */
>> + /* { dg-message "\\(1\\) Casting a 42 byte buffer to 'int \\*'
>> leaves
>> 2 trailing bytes" "" { target *-*-* } malloc } */
>> +}
>> +
>> +void test_3 (void)
>> +{
>> + void *ptr = malloc (21 * sizeof (short));
>> + short *sptr = (short *)ptr;
>> + free (sptr);
>> +}
>> +
>> +void test_4 (void)
>> +{
>> + void *ptr = malloc (21 * sizeof (short)); /* { dg-message } */
>> + int *iptr = (int *)ptr; /* { dg-line assign } */
>> + free (iptr);
>> +
>> + /* { dg-warning "Allocated buffer size is not a multiple of the
>> pointee's size" "" { target *-*-* } assign } */
>> + /* { dg-message "\\(2\\) Casting 'ptr' to 'int \\*' leaves 2
>> trailing
>> bytes" "" { target *-*-* } assign } */
>> +}
>> +
>> +struct s {
>> + int i;
>> +};
>> +
>> +void test_5 (void)
>> +{
>> + struct s *ptr = malloc (5 * sizeof (struct s));
>> + free (ptr);
>> +}
>> +
>> +void test_6 (void)
>> +{
>> + long *ptr = malloc (5 * sizeof (struct s)); /* { dg-line malloc6
>> } */
>> + free (ptr);
>> +
>> + /* { dg-warning "" "" { target *-*-* } malloc6 } */
>> + /* { dg-message "" "" { target *-*-* } malloc6 } */
>> +}
>> diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
>> b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
>> new file mode 100644
>> index 00000000000..e66d2793f13
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
>> @@ -0,0 +1,44 @@
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +
>> +/* Tests with symbolic buffer sizes */
>> +
>> +void test_1 (void)
>> +{
>> + int n;
>> + scanf("%i", &n);
>> + short *ptr = malloc (n * sizeof(short));
>> + free (ptr);
>> +}
>> +
>> +void test_2 (void)
>> +{
>> + int n;
>> + scanf("%i", &n);
>> + int *ptr = malloc (n * sizeof (short)); /* { dg-line malloc } */
>> + free (ptr);
>> +
>> + /* { dg-warning "Allocated buffer size is not a multiple of the
>> pointee's size" "" { target *-*-* } malloc } */
>> + /* { dg-message "\\(1\\) Allocation is incompatible with 'int
>> \\*'"
>> "" { target *-*-* } malloc } */
>> +}
>> +
>> +void test_3 (void)
>> +{
>> + int n;
>> + scanf("%i", &n);
>> + void *ptr = malloc (n * sizeof (short));
>> + short *sptr = (short *)ptr;
>> + free (sptr);
>> +}
>> +
>> +void test_4 (void)
>> +{
>> + int n;
>> + scanf("%i", &n);
>> + void *ptr = malloc (n * sizeof (short)); /* { dg-message } */
>> + int *iptr = (int *)ptr; /* { dg-line assign } */
>> + free (iptr);
>> +
>> + /* { dg-warning "Allocated buffer size is not a multiple of the
>> pointee's size" "" { target *-*-* } assign } */
>> + /* { dg-message "\\(2\\) 'ptr' is incompatible with 'int \\*';
>> either
>> the allocated size at \\(1\\)" "" { target *-*-* } assign } */
>> +}
>> diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c
>> b/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c
>> new file mode 100644
>> index 00000000000..dafc0e73c63
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c
>> @@ -0,0 +1,48 @@
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +
>> +/* CWE-131 example 5 */
>> +void test_1(void)
>> +{
>> + int *id_sequence = (int *) malloc (3); /* { dg-line malloc1 } */
>> + if (id_sequence == NULL) exit (1);
>> +
>> + id_sequence[0] = 13579;
>> + id_sequence[1] = 24680;
>> + id_sequence[2] = 97531;
>> +
>> + free (id_sequence);
>> +
>> + /* { dg-warning "" "" { target *-*-* } malloc1 } */
>> + /* { dg-message "" "" { target *-*-* } malloc1 } */
>> +}
>> +
>> +void test_2(void)
>> +{
>> + int *ptr = malloc (10 + sizeof(int)); /* { dg-line malloc2 } */
>> + free (ptr);
>> +
>> + /* { dg-warning "" "" { target *-*-* } malloc2 } */
>> + /* { dg-message "" "" { target *-*-* } malloc2 } */
>> +}
>> +
>> +void test_3(void)
>> +{
>> + int n;
>> + scanf("%i", &n);
>> + int *ptr = malloc (n + sizeof (int)); /* { dg-line malloc3 } */
>> + free (ptr);
>> +
>> + /* { dg-warning "" "" { target *-*-* } malloc3 } */
>> + /* { dg-message "" "" { target *-*-* } malloc3 } */
>> +}
>> +
>> +void test_4(void)
>> +{
>> + int n;
>> + scanf("%i", &n);
>> + int m;
>> + scanf("%i", &m);
>> + int *ptr = malloc ((n + m) * sizeof (int));
>> + free (ptr);
>> +}
>> diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c
>> b/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c
>> new file mode 100644
>> index 00000000000..4c2b31d6e0a
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c
>> @@ -0,0 +1,39 @@
>> +#include <stddef.h>
>> +#include <stdlib.h>
>> +
>> +/* Flow warnings */
>> +
>> +void *create_buffer(int n)
>> +{
>> + return malloc(n);
>> +}
>> +
>> +void test_1(void)
>> +{
>> + // FIXME
>> + int *buf = create_buffer(42); /* { dg-warning "" "" { xfail *-*-*
>> } }
>> */
>> + free (buf);
>> +}
>> +
>> +void test_2(void)
>> +{
>> + void *buf = create_buffer(42); /* { dg-message } */
>> + int *ibuf = buf; /* { dg-line assign2 } */
>> + free (ibuf);
>> +
>> + /* { dg-warning "" "" { target *-*-* } assign2 } */
>> + /* { dg-message "" "" { target *-*-* } assign2 } */
>> +}
>> +
>> +void test_3(void)
>> +{
>> + void *buf = malloc(42); /* { dg-message } */
>> + if (buf != NULL) /* { dg-message } */
>> + {
>> + int *ibuf = buf; /* { dg-line assign3 } */
>> + free (ibuf);
>> + }
>> +
>> + /* { dg-warning "" "" { target *-*-* } assign3 } */
>> + /* { dg-message "" "" { target *-*-* } assign3 } */
>> +}
>> diff --git a/gcc/testsuite/gcc.dg/analyzer/attr-malloc-6.c
>> b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-6.c
>> index bd28107d0d7..809ee88cf07 100644
>> --- a/gcc/testsuite/gcc.dg/analyzer/attr-malloc-6.c
>> +++ b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-6.c
>> @@ -1,7 +1,9 @@
>> +/* { dg-additional-options -Wno-analyzer-allocation-size } */
>> /* Adapted from gcc.dg/Wmismatched-dealloc.c. */
>>
>> #define A(...) __attribute__ ((malloc (__VA_ARGS__)))
>>
>> +struct FILE {};
>> typedef struct FILE FILE;
>> typedef __SIZE_TYPE__ size_t;
>>
>> diff --git a/gcc/testsuite/gcc.dg/analyzer/capacity-1.c
>> b/gcc/testsuite/gcc.dg/analyzer/capacity-1.c
>> index 2d124833296..94f569e390b 100644
>> --- a/gcc/testsuite/gcc.dg/analyzer/capacity-1.c
>> +++ b/gcc/testsuite/gcc.dg/analyzer/capacity-1.c
>> @@ -89,8 +89,11 @@ struct s
>> static struct s * __attribute__((noinline))
>> alloc_s (size_t num)
>> {
>> - struct s *p = malloc (sizeof(struct s) + num);
>> + struct s *p = malloc (sizeof(struct s) + num); /* { dg-line
>> malloc }
>> */
>> return p;
>> +
>> + /* { dg-warning "" "" { target *-*-* } malloc } */
>> + /* { dg-message "" "" { target *-*-* } malloc } */
>> }
>>
>> struct s *
>> diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-4.c
>> b/gcc/testsuite/gcc.dg/analyzer/malloc-4.c
>> index 908bb28ee50..0ca94250ba2 100644
>> --- a/gcc/testsuite/gcc.dg/analyzer/malloc-4.c
>> +++ b/gcc/testsuite/gcc.dg/analyzer/malloc-4.c
>> @@ -1,9 +1,9 @@
>> -/* { dg-additional-options "-Wno-incompatible-pointer-types" } */
>> +/* { dg-additional-options "-Wno-incompatible-pointer-types
>> -Wno-analyzer-allocation-size" } */
>>
>> #include <stdlib.h>
>>
>> -struct foo;
>> -struct bar;
>> +struct foo {};
>> +struct bar {};
>> void *hv (struct foo **tm)
>> {
>> void *p = __builtin_malloc (4);
>> diff --git a/gcc/testsuite/gcc.dg/analyzer/pr96639.c
>> b/gcc/testsuite/gcc.dg/analyzer/pr96639.c
>> index 02ca3f084a2..6f365c3cb5d 100644
>> --- a/gcc/testsuite/gcc.dg/analyzer/pr96639.c
>> +++ b/gcc/testsuite/gcc.dg/analyzer/pr96639.c
>> @@ -1,3 +1,5 @@
>> +/* { dg-additional-options -Wno-analyzer-allocation-size } */
>> +
>> void *calloc (__SIZE_TYPE__, __SIZE_TYPE__);
>>
>> int
>> --
>> 2.36.1
>>
>>
>>
next prev parent reply other threads:[~2022-06-17 19:23 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-17 15:54 Tim Lange
2022-06-17 17:15 ` Prathamesh Kulkarni
2022-06-17 19:23 ` Tim Lange [this message]
2022-06-17 21:39 ` David Malcolm
2022-06-17 17:48 ` David Malcolm
2022-06-17 20:23 ` Tim Lange
2022-06-17 22:13 ` David Malcolm
2022-06-21 20:00 ` Tim Lange
2022-06-21 23:16 ` David Malcolm
2022-06-22 14:57 ` Tim Lange
2022-06-22 18:23 ` David Malcolm
2022-06-17 18:34 ` [RFC] analyzer: add " Tim Lange
2022-06-29 15:39 ` [PATCH v2] analyzer: add allocation size checker Tim Lange
2022-06-29 17:39 ` David Malcolm
2022-06-30 20:40 ` Tim Lange
2022-06-30 22:11 ` [PATCH v3] analyzer: add allocation size checker [PR105900] Tim Lange
2022-06-30 22:47 ` David Malcolm
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=A7ZMDR.7J1GGPCTKYH3@tim-lange.me \
--to=mail@tim-lange.me \
--cc=dmalcolm@redhat.com \
--cc=gcc@gcc.gnu.org \
--cc=prathamesh.kulkarni@linaro.org \
/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).