analyzer: Add support of placement new and improved operator new [PR105948, PR94355]
Checks
Commit Message
From: benjamin priour <priour.be@gmail.com>
Hi,
Succesfully regstrapped off trunk 7f2ed06ddc825e8a4e0edfd1d66b5156e6dc1d34
on x86_64-linux-gnu.
Is it OK for trunk ?
Thanks,
Benjamin.
Patch below.
---
Fixed spurious possibly-NULL warning always tagging along throwing
operator new despite it never returning NULL.
Now operator new is correctly recognized as possibly returning NULL
if and only if it is non-throwing or exceptions have been disabled.
Different standard signatures of operator new are now properly
recognized.
Added support of placement new, so that it is now properly recognized,
and a 'heap_allocated' region is no longer created for it.
Placement new size is also checked and a 'Wanalyzer-allocation-size'
is emitted when relevant, as well as always a 'Wanalyzer-out-of-bounds'.
'operator new' non-throwing variants are detected y checking the types
of the parameters.
Indeed, in a call to new (std::nothrow) () the chosen overload
has signature 'operator new (void*, std::nothrow_t&)', where the second
parameter is a reference. In a placement new, the second parameter will
always be a void pointer.
Prior to this patch, some buffers first allocated with 'new', then deleted
an thereafter used would result in a 'Wanalyzer-user-after-free'
warning. However the wording was "use after 'free'" instead of the
expected "use after 'delete'".
This patch fixes this by introducing a new kind of poisoned value,
namely POISON_KIND_DELETED.
Due to how the analyzer sees calls to non-throwing variants of
operator new, dereferencing a pointer freshly allocated in this fashion
caused both a 'Wanalyzer-use-of-uninitialized-value' and a
'Wanalyzer-null-dereference' to be emitted, while only the latter was
relevant. As a result, 'null-dereference' now supersedes
'use-of-uninitialized'.
Signed-off-by: benjamin priour <vultkayn@gcc.gnu.org>
gcc/analyzer/ChangeLog:
PR analyzer/105948
PR analyzer/94355
* analyzer.h (is_placement_new_p): New declaration.
* call-details.cc
(call_details::maybe_get_arg_region): New function.
Returns the region of the argument at given index if possible.
* call-details.h: Declaration of the above function.
* kf-lang-cp.cc (is_placement_new_p): Returns true if the gcall
is recognized as a placement new.
(kf_operator_delete::impl_call_post): Unbinding a region and its
descendents now poisons with POISON_KIND_DELETED.
(register_known_functions_lang_cp): Known function "operator
delete" is now registered only once independently of its number of
arguments.
* region-model.cc (region_model::eval_condition): Now
recursively calls itself if any of the operand is wrapped in a
cast.
* sm-malloc.cc (malloc_state_machine::on_stmt):
Add placement new recognition.
* svalue.cc (poison_kind_to_str): Wording for the new PK.
* svalue.h (enum poison_kind): Add value POISON_KIND_DELETED.
gcc/testsuite/ChangeLog:
PR analyzer/105948
PR analyzer/94355
* g++.dg/analyzer/out-of-bounds-placement-new.C: Added a directive.
* g++.dg/analyzer/placement-new.C: Added tests.
* g++.dg/analyzer/new-2.C: New test.
* g++.dg/analyzer/noexcept-new.C: New test.
* g++.dg/analyzer/placement-new-size.C: New test.
---
gcc/analyzer/analyzer.h | 1 +
gcc/analyzer/call-details.cc | 11 ++
gcc/analyzer/call-details.h | 1 +
gcc/analyzer/kf-lang-cp.cc | 117 +++++++++++++++---
gcc/analyzer/region-model.cc | 36 ++++++
gcc/analyzer/sm-malloc.cc | 37 ++++--
gcc/analyzer/svalue.cc | 2 +
gcc/analyzer/svalue.h | 3 +
gcc/testsuite/g++.dg/analyzer/new-2.C | 70 +++++++++++
gcc/testsuite/g++.dg/analyzer/noexcept-new.C | 48 +++++++
.../analyzer/out-of-bounds-placement-new.C | 2 +-
.../g++.dg/analyzer/placement-new-size.C | 27 ++++
gcc/testsuite/g++.dg/analyzer/placement-new.C | 90 +++++++++++++-
13 files changed, 417 insertions(+), 28 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/analyzer/new-2.C
create mode 100644 gcc/testsuite/g++.dg/analyzer/noexcept-new.C
create mode 100644 gcc/testsuite/g++.dg/analyzer/placement-new-size.C
Comments
On Fri, 2023-09-01 at 00:04 +0200, priour.be@gmail.com wrote:
> Hi,
>
> Succesfully regstrapped off trunk 7f2ed06ddc825e8a4e0edfd1d66b5156e6dc1d34
> on x86_64-linux-gnu.
>
> Is it OK for trunk ?
Hi Benjamin.
Thanks for the patch. It's OK as-is, but it doesn't cover every
case...
[...snip...]
> diff --git a/gcc/analyzer/call-details.cc b/gcc/analyzer/call-details.cc
> index 66fb0fe871e..8d60e928b15 100644
> --- a/gcc/analyzer/call-details.cc
> +++ b/gcc/analyzer/call-details.cc
> @@ -295,6 +295,17 @@ call_details::get_arg_svalue (unsigned idx) const
> return m_model->get_rvalue (arg, m_ctxt);
> }
>
> +/* If argument IDX's svalue at the callsite is a region_svalue,
> + return the region it points to.
> + Otherwise return NULL. */
> +
> +const region *
> +call_details::maybe_get_arg_region (unsigned idx) const
> +{
> + const svalue *sval = get_arg_svalue (idx);
> + return sval->maybe_get_region ();
> +}
> +
Is this the correct thing to be doing? It's used in the following...
[...snip...]
> diff --git a/gcc/analyzer/kf-lang-cp.cc b/gcc/analyzer/kf-lang-cp.cc
> index 393b4f25e79..4450892dfa2 100644
> --- a/gcc/analyzer/kf-lang-cp.cc
> +++ b/gcc/analyzer/kf-lang-cp.cc
[...snip...]
> @@ -54,28 +90,75 @@ public:
> region_model *model = cd.get_model ();
> region_model_manager *mgr = cd.get_manager ();
> const svalue *size_sval = cd.get_arg_svalue (0);
> - const region *new_reg
> - = model->get_or_create_region_for_heap_alloc (size_sval, cd.get_ctxt ());
> - if (cd.get_lhs_type ())
> + region_model_context *ctxt = cd.get_ctxt ();
> + const gcall *call = cd.get_call_stmt ();
> +
> + /* If the call was actually a placement new, check that accessing
> + the buffer lhs is placed into does not result in out-of-bounds. */
> + if (is_placement_new_p (call))
> {
> - const svalue *ptr_sval
> - = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
> - cd.maybe_set_lhs (ptr_sval);
> + const region *ptr_reg = cd.maybe_get_arg_region (1);
> + if (ptr_reg && cd.get_lhs_type ())
> + {
...which will only fire if arg 1 is a region_svalue. This won't
trigger if you have e.g. a binop_svalue for pointer arithmetic.
What happens e.g. for this one-off-the-end bug:
void *p = malloc (4);
if (!p)
return;
int32_t *i = ::new (p + 1) int32_t;
*i = 42;
So maybe call_details::maybe_get_arg_region should instead be:
/* Return the region that argument IDX points to. */
const region *
call_details::deref_ptr_arg (unsigned idx) const
{
const svalue *ptr_sval = get_arg_svalue (idx);
return m_model->deref_rvalue (ptr_sval, get_arg_tree (idx), m_ctxt);
}
(caveat: I didn't test this)
> + const region *base_reg = ptr_reg->get_base_region ();
> + const svalue *num_bytes_sval = cd.get_arg_svalue (0);
> + const region *sized_new_reg
> + = mgr->get_sized_region (base_reg,
> + cd.get_lhs_type (),
> + num_bytes_sval);
Why do you use the base_reg here, rather than just ptr_reg?
In the example above, the *(p + 1) has base region
heap_allocated_region, but the ptr_reg is one byte higher; hence
check_region_for_write of 4 bytes ought to detect a problem with
writing 4 bytes to *(p + 1), but wouldn't complain about the write to
*p.
...assuming that I'm reading this code correctly.
> + model->check_region_for_write (sized_new_reg,
> + nullptr,
> + ctxt);
> + const svalue *ptr_sval
> + = mgr->get_ptr_svalue (cd.get_lhs_type (), sized_new_reg);
> + cd.maybe_set_lhs (ptr_sval);
> + }
> + }
[...snip...]
The patch is OK for trunk as is; but please can you look into the
above.
If the above is a problem, you can either do another version of the
patch, or do it as a followup patch (whichever you're more comfortable
with, but it might be best to get the patch into trunk as-is, given
that the GSoC period is nearly over).
Thanks
Dave
Hi David,
On Fri, Sep 1, 2023 at 1:59 AM David Malcolm <dmalcolm@redhat.com> wrote:
> On Fri, 2023-09-01 at 00:04 +0200, priour.be@gmail.com wrote:
>
>
[..snip..]
> ...which will only fire if arg 1 is a region_svalue. This won't
> trigger if you have e.g. a binop_svalue for pointer arithmetic.
>
> What happens e.g. for this one-off-the-end bug:
>
> void *p = malloc (4);
> if (!p)
> return;
> int32_t *i = ::new (p + 1) int32_t;
> *i = 42;
>
> So maybe call_details::maybe_get_arg_region should instead be:
>
> /* Return the region that argument IDX points to. */
>
> const region *
> call_details::deref_ptr_arg (unsigned idx) const
> {
> const svalue *ptr_sval = get_arg_svalue (idx);
> return m_model->deref_rvalue (ptr_sval, get_arg_tree (idx), m_ctxt);
> }
>
> (caveat: I didn't test this)
>
> > + const region *base_reg = ptr_reg->get_base_region ();
> > + const svalue *num_bytes_sval = cd.get_arg_svalue (0);
> > + const region *sized_new_reg
> > + = mgr->get_sized_region (base_reg,
> > + cd.get_lhs_type (),
> > + num_bytes_sval);
>
> Why do you use the base_reg here, rather than just ptr_reg?
>
> In the example above, the *(p + 1) has base region
> heap_allocated_region, but the ptr_reg is one byte higher; hence
> check_region_for_write of 4 bytes ought to detect a problem with
> writing 4 bytes to *(p + 1), but wouldn't complain about the write to
> *p.
>
> ...assuming that I'm reading this code correctly.
>
> > + model->check_region_for_write (sized_new_reg,
> > + nullptr,
> > + ctxt);
> > + const svalue *ptr_sval
> > + = mgr->get_ptr_svalue (cd.get_lhs_type (), sized_new_reg);
> > + cd.maybe_set_lhs (ptr_sval);
> > + }
> > + }
>
> [...snip...]
>
> The patch is OK for trunk as is; but please can you look into the
> above.
>
>
Thanks for the test case David, it exposed a missing heap-based over write
when on the placement new statement.
I've updated the code as per your suggestions, and it now works properly.
> If the above is a problem, you can either do another version of the
> patch, or do it as a followup patch (whichever you're more comfortable
> with, but it might be best to get the patch into trunk as-is, given
> that the GSoC period is nearly over).
>
> Thanks
> Dave
>
>
I will update the patch and regstrap it, so that it is done at once.
I've compared the new test case to a "C" version of it, resulting in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111266
I will attempt to fix it while I'm regstrapping everything else,
I still have 4 patches in queue.
It will give me a brief break from transitioning the tests :)
Thanks for the review,
Benjamin.
Patch has been updated as per your suggestions and successfully regstrapped
on x86_64-linux-gnu.
call_details::maybe_get_arg_region is now
/* If argument IDX's svalue at the callsite is of pointer type,
return the region it points to.
Otherwise return NULL. */
const region *
call_details::deref_ptr_arg (unsigned idx) const
{
const svalue *ptr_sval = get_arg_svalue (idx);
return m_model->deref_rvalue (ptr_sval, get_arg_tree (idx), m_ctxt);
}
New test is
+
+void test_binop ()
+{
+ char *p = (char *) malloc (4);
+ if (!p)
+ return;
+ int32_t *i = ::new (p + 1) int32_t; /* { dg-warning "heap-based buffer
overflow" } */
+ *i = 42; /* { dg-warning "heap-based buffer overflow" } */
+ free (p);
+}
Is it OK for trunk ?
I didn't resend the whole patch as it otherwise was OK.
Thanks,
Benjamin.
On Fri, Sep 1, 2023 at 12:07 PM Benjamin Priour <priour.be@gmail.com> wrote:
> Hi David,
>
> On Fri, Sep 1, 2023 at 1:59 AM David Malcolm <dmalcolm@redhat.com> wrote:
>
>> On Fri, 2023-09-01 at 00:04 +0200, priour.be@gmail.com wrote:
>>
>>
> [..snip..]
>
>
>> ...which will only fire if arg 1 is a region_svalue. This won't
>> trigger if you have e.g. a binop_svalue for pointer arithmetic.
>>
>> What happens e.g. for this one-off-the-end bug:
>>
>> void *p = malloc (4);
>> if (!p)
>> return;
>> int32_t *i = ::new (p + 1) int32_t;
>> *i = 42;
>>
>> So maybe call_details::maybe_get_arg_region should instead be:
>>
>> /* Return the region that argument IDX points to. */
>>
>> const region *
>> call_details::deref_ptr_arg (unsigned idx) const
>> {
>> const svalue *ptr_sval = get_arg_svalue (idx);
>> return m_model->deref_rvalue (ptr_sval, get_arg_tree (idx), m_ctxt);
>> }
>>
>> (caveat: I didn't test this)
>>
>> > + const region *base_reg = ptr_reg->get_base_region ();
>> > + const svalue *num_bytes_sval = cd.get_arg_svalue (0);
>> > + const region *sized_new_reg
>> > + = mgr->get_sized_region (base_reg,
>> > + cd.get_lhs_type (),
>> > + num_bytes_sval);
>>
>> Why do you use the base_reg here, rather than just ptr_reg?
>>
>> In the example above, the *(p + 1) has base region
>> heap_allocated_region, but the ptr_reg is one byte higher; hence
>> check_region_for_write of 4 bytes ought to detect a problem with
>> writing 4 bytes to *(p + 1), but wouldn't complain about the write to
>> *p.
>>
>> ...assuming that I'm reading this code correctly.
>>
>> > + model->check_region_for_write (sized_new_reg,
>> > + nullptr,
>> > + ctxt);
>> > + const svalue *ptr_sval
>> > + = mgr->get_ptr_svalue (cd.get_lhs_type (), sized_new_reg);
>> > + cd.maybe_set_lhs (ptr_sval);
>> > + }
>> > + }
>>
>> [...snip...]
>>
>> The patch is OK for trunk as is; but please can you look into the
>> above.
>>
>>
> Thanks for the test case David, it exposed a missing heap-based over write
> when on the placement new statement.
> I've updated the code as per your suggestions, and it now works properly.
>
>
>> If the above is a problem, you can either do another version of the
>> patch, or do it as a followup patch (whichever you're more comfortable
>> with, but it might be best to get the patch into trunk as-is, given
>> that the GSoC period is nearly over).
>>
>> Thanks
>> Dave
>>
>>
> I will update the patch and regstrap it, so that it is done at once.
> I've compared the new test case to a "C" version of it, resulting in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111266
>
> I will attempt to fix it while I'm regstrapping everything else,
> I still have 4 patches in queue.
> It will give me a brief break from transitioning the tests :)
>
> Thanks for the review,
> Benjamin.
>
On Fri, 2023-09-01 at 16:48 +0200, Benjamin Priour wrote:
> Patch has been updated as per your suggestions and successfully
> regstrapped
> on x86_64-linux-gnu.
>
> call_details::maybe_get_arg_region is now
> /* If argument IDX's svalue at the callsite is of pointer type,
> return the region it points to.
> Otherwise return NULL. */
>
> const region *
> call_details::deref_ptr_arg (unsigned idx) const
> {
> const svalue *ptr_sval = get_arg_svalue (idx);
> return m_model->deref_rvalue (ptr_sval, get_arg_tree (idx),
> m_ctxt);
> }
>
>
> New test is
>
> +
> +void test_binop ()
> +{
> + char *p = (char *) malloc (4);
> + if (!p)
> + return;
> + int32_t *i = ::new (p + 1) int32_t; /* { dg-warning "heap-based
> buffer
> overflow" } */
> + *i = 42; /* { dg-warning "heap-based buffer overflow" } */
> + free (p);
> +}
>
> Is it OK for trunk ?
> I didn't resend the whole patch as it otherwise was OK.
Yes, thanks.
Dave
Hi Benjanmin,
On Fri, 1 Sept 2023 at 17:45, David Malcolm via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:
> On Fri, 2023-09-01 at 16:48 +0200, Benjamin Priour wrote:
> > Patch has been updated as per your suggestions and successfully
> > regstrapped
> > on x86_64-linux-gnu.
> >
The new testcase placement-new-size.C fails on aarch64:
placement-new-size.C:10:3: error: 'int8_t' was not declared in this scope;
did you mean 'wint_t'?
placement-new-size.C:11:3: error: 'int64_t' was not declared in this scope
placement-new-size.C:11:12: error: 'lp' was not declared in this scope
placement-new-size.C:11:23: error: 's' was not declared in this scope
placement-new-size.C:11:26: error: 'int64_t' does not name a type
placement-new-size.C:34:3: error: 'int32_t' was not declared in this scope
placement-new-size.C:34:12: error: 'i' was not declared in this scope
placement-new-size.C:34:30: error: 'int32_t' does not name a type
I suspect you should include <stdint.h> (instead of stdlib.h)
Thanks,
Christophe
> > call_details::maybe_get_arg_region is now
> > /* If argument IDX's svalue at the callsite is of pointer type,
> > return the region it points to.
> > Otherwise return NULL. */
> >
> > const region *
> > call_details::deref_ptr_arg (unsigned idx) const
> > {
> > const svalue *ptr_sval = get_arg_svalue (idx);
> > return m_model->deref_rvalue (ptr_sval, get_arg_tree (idx),
> > m_ctxt);
> > }
> >
> >
> > New test is
> >
> > +
> > +void test_binop ()
> > +{
> > + char *p = (char *) malloc (4);
> > + if (!p)
> > + return;
> > + int32_t *i = ::new (p + 1) int32_t; /* { dg-warning "heap-based
> > buffer
> > overflow" } */
> > + *i = 42; /* { dg-warning "heap-based buffer overflow" } */
> > + free (p);
> > +}
> >
> > Is it OK for trunk ?
> > I didn't resend the whole patch as it otherwise was OK.
>
> Yes, thanks.
>
> Dave
>
>
@@ -423,6 +423,7 @@ extern bool is_std_named_call_p (const_tree fndecl, const char *funcname,
const gcall *call, unsigned int num_args);
extern bool is_setjmp_call_p (const gcall *call);
extern bool is_longjmp_call_p (const gcall *call);
+extern bool is_placement_new_p (const gcall *call);
extern const char *get_user_facing_name (const gcall *call);
@@ -295,6 +295,17 @@ call_details::get_arg_svalue (unsigned idx) const
return m_model->get_rvalue (arg, m_ctxt);
}
+/* If argument IDX's svalue at the callsite is a region_svalue,
+ return the region it points to.
+ Otherwise return NULL. */
+
+const region *
+call_details::maybe_get_arg_region (unsigned idx) const
+{
+ const svalue *sval = get_arg_svalue (idx);
+ return sval->maybe_get_region ();
+}
+
/* Attempt to get the string literal for argument IDX, or return NULL
otherwise.
For use when implementing "__analyzer_*" functions that take
@@ -61,6 +61,7 @@ public:
tree get_arg_tree (unsigned idx) const;
tree get_arg_type (unsigned idx) const;
const svalue *get_arg_svalue (unsigned idx) const;
+ const region *maybe_get_arg_region (unsigned idx) const;
const char *get_arg_string_literal (unsigned idx) const;
tree get_fndecl_for_call () const;
@@ -35,6 +35,38 @@ along with GCC; see the file COPYING3. If not see
#if ENABLE_ANALYZER
+/* Return true if CALL is a non-allocating operator new or operator new []
+ that contains no user-defined args, i.e. having any signature of:
+
+ - void* operator new (std::size_t count, void* ptr);
+ - void* operator new[] (std::size_t count, void* ptr);
+
+ See https://en.cppreference.com/w/cpp/memory/new/operator_new. */
+
+bool is_placement_new_p (const gcall *call)
+{
+ gcc_assert (call);
+ tree fndecl = gimple_call_fndecl (call);
+
+ if (!fndecl || TREE_CODE (TREE_TYPE (fndecl)) == METHOD_TYPE)
+ /* Give up on overloaded operator new. */
+ return false;
+
+ if (!is_named_call_p (fndecl, "operator new", call, 2)
+ && !is_named_call_p (fndecl, "operator new []", call, 2))
+ return false;
+
+ /* We must distinguish between an allocating non-throwing new
+ and a non-allocating new.
+
+ The former might have one of the following signatures :
+ void* operator new (std::size_t count, const std::nothrow_t& tag);
+ void* operator new[] (std::size_t count, const std::nothrow_t& tag);
+ Whereas a placement new would take a pointer. */
+ tree arg1_type = TREE_CHAIN (TYPE_ARG_TYPES (TREE_TYPE (fndecl)));
+ return TREE_CODE (TREE_VALUE (arg1_type)) == POINTER_TYPE;
+}
+
namespace ana {
/* Implementations of specific functions. */
@@ -46,7 +78,11 @@ class kf_operator_new : public known_function
public:
bool matches_call_types_p (const call_details &cd) const final override
{
- return cd.num_args () == 1;
+ return (cd.num_args () == 1
+ && cd.arg_is_size_p (0))
+ || (cd.num_args () == 2
+ && cd.arg_is_size_p (0)
+ && POINTER_TYPE_P (cd.get_arg_type (1)));
}
void impl_call_pre (const call_details &cd) const final override
@@ -54,28 +90,75 @@ public:
region_model *model = cd.get_model ();
region_model_manager *mgr = cd.get_manager ();
const svalue *size_sval = cd.get_arg_svalue (0);
- const region *new_reg
- = model->get_or_create_region_for_heap_alloc (size_sval, cd.get_ctxt ());
- if (cd.get_lhs_type ())
+ region_model_context *ctxt = cd.get_ctxt ();
+ const gcall *call = cd.get_call_stmt ();
+
+ /* If the call was actually a placement new, check that accessing
+ the buffer lhs is placed into does not result in out-of-bounds. */
+ if (is_placement_new_p (call))
{
- const svalue *ptr_sval
- = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
- cd.maybe_set_lhs (ptr_sval);
+ const region *ptr_reg = cd.maybe_get_arg_region (1);
+ if (ptr_reg && cd.get_lhs_type ())
+ {
+ const region *base_reg = ptr_reg->get_base_region ();
+ const svalue *num_bytes_sval = cd.get_arg_svalue (0);
+ const region *sized_new_reg
+ = mgr->get_sized_region (base_reg,
+ cd.get_lhs_type (),
+ num_bytes_sval);
+ model->check_region_for_write (sized_new_reg,
+ nullptr,
+ ctxt);
+ const svalue *ptr_sval
+ = mgr->get_ptr_svalue (cd.get_lhs_type (), sized_new_reg);
+ cd.maybe_set_lhs (ptr_sval);
+ }
+ }
+ /* If the call is an allocating new, then create a heap allocated
+ region. */
+ else
+ {
+ const region *new_reg
+ = model->get_or_create_region_for_heap_alloc (size_sval, ctxt);
+ if (cd.get_lhs_type ())
+ {
+ const svalue *ptr_sval
+ = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
+ cd.maybe_set_lhs (ptr_sval);
+ }
+ }
+ }
+
+ void impl_call_post (const call_details &cd) const final override
+ {
+ region_model *model = cd.get_model ();
+ region_model_manager *mgr = cd.get_manager ();
+ tree callee_fndecl = cd.get_fndecl_for_call ();
+ region_model_context *ctxt = cd.get_ctxt ();
+
+ /* If the call is guaranteed to return nonnull
+ then add a nonnull constraint to the allocated region. */
+ if (!TREE_NOTHROW (callee_fndecl) && flag_exceptions)
+ {
+ const svalue *null_sval
+ = mgr->get_or_create_null_ptr (cd.get_lhs_type ());
+ const svalue *result
+ = model->get_store_value (cd.get_lhs_region (), ctxt);
+ model->add_constraint (result, NE_EXPR, null_sval, ctxt);
}
}
};
-/* Handler for "operator delete", both the sized and unsized variants
- (2 arguments and 1 argument respectively), and for "operator delete []" */
+/* Handler for "operator delete" and for "operator delete []",
+ both the sized and unsized variants
+ (2 arguments and 1 argument respectively). */
class kf_operator_delete : public known_function
{
public:
- kf_operator_delete (unsigned num_args) : m_num_args (num_args) {}
-
bool matches_call_types_p (const call_details &cd) const final override
{
- return cd.num_args () == m_num_args;
+ return cd.num_args () == 1 or cd.num_args () == 2;
}
void impl_call_post (const call_details &cd) const final override
@@ -86,12 +169,11 @@ public:
{
/* If the ptr points to an underlying heap region, delete it,
poisoning pointers. */
- model->unbind_region_and_descendents (freed_reg, POISON_KIND_FREED);
+ model->unbind_region_and_descendents (freed_reg,
+ POISON_KIND_DELETED);
}
}
-private:
- unsigned m_num_args;
};
/* Populate KFM with instances of known functions relating to C++. */
@@ -101,9 +183,8 @@ register_known_functions_lang_cp (known_function_manager &kfm)
{
kfm.add ("operator new", make_unique<kf_operator_new> ());
kfm.add ("operator new []", make_unique<kf_operator_new> ());
- kfm.add ("operator delete", make_unique<kf_operator_delete> (1));
- kfm.add ("operator delete", make_unique<kf_operator_delete> (2));
- kfm.add ("operator delete []", make_unique<kf_operator_delete> (1));
+ kfm.add ("operator delete", make_unique<kf_operator_delete> ());
+ kfm.add ("operator delete []", make_unique<kf_operator_delete> ());
}
} // namespace ana
@@ -499,6 +499,7 @@ public:
case POISON_KIND_UNINIT:
return OPT_Wanalyzer_use_of_uninitialized_value;
case POISON_KIND_FREED:
+ case POISON_KIND_DELETED:
return OPT_Wanalyzer_use_after_free;
case POISON_KIND_POPPED_STACK:
return OPT_Wanalyzer_use_of_pointer_in_stale_stack_frame;
@@ -531,6 +532,15 @@ public:
m_expr);
}
break;
+ case POISON_KIND_DELETED:
+ {
+ diagnostic_metadata m;
+ m.add_cwe (416); /* "CWE-416: Use After Free". */
+ return warning_meta (rich_loc, m, get_controlling_option (),
+ "use after %<delete%> of %qE",
+ m_expr);
+ }
+ break;
case POISON_KIND_POPPED_STACK:
{
/* TODO: which CWE? */
@@ -555,6 +565,9 @@ public:
case POISON_KIND_FREED:
return ev.formatted_print ("use after %<free%> of %qE here",
m_expr);
+ case POISON_KIND_DELETED:
+ return ev.formatted_print ("use after %<delete%> of %qE here",
+ m_expr);
case POISON_KIND_POPPED_STACK:
return ev.formatted_print
("dereferencing pointer %qE to within stale stack frame",
@@ -4191,6 +4204,29 @@ region_model::eval_condition (const svalue *lhs,
}
}
+ /* Attempt to unwrap cast if there is one, and the types match. */
+ tree lhs_type = lhs->get_type ();
+ tree rhs_type = rhs->get_type ();
+ if (lhs_type && rhs_type)
+ {
+ const unaryop_svalue *lhs_un_op = dyn_cast <const unaryop_svalue *> (lhs);
+ const unaryop_svalue *rhs_un_op = dyn_cast <const unaryop_svalue *> (rhs);
+ if (lhs_un_op && CONVERT_EXPR_CODE_P (lhs_un_op->get_op ())
+ && rhs_un_op && CONVERT_EXPR_CODE_P (rhs_un_op->get_op ())
+ && lhs_type == rhs_type)
+ return eval_condition (lhs_un_op->get_arg (),
+ op,
+ rhs_un_op->get_arg ());
+
+ else if (lhs_un_op && CONVERT_EXPR_CODE_P (lhs_un_op->get_op ())
+ && lhs_type == rhs_type)
+ return eval_condition (lhs_un_op->get_arg (), op, rhs);
+
+ else if (rhs_un_op && CONVERT_EXPR_CODE_P (rhs_un_op->get_op ())
+ && lhs_type == rhs_type)
+ return eval_condition (lhs, op, rhs_un_op->get_arg ());
+ }
+
/* Otherwise, try constraints.
Cast to const to ensure we don't change the constraint_manager as we
do this (e.g. by creating equivalence classes). */
@@ -759,7 +759,7 @@ public:
override
{
if (change.m_old_state == m_sm.get_start_state ()
- && unchecked_p (change.m_new_state))
+ && (unchecked_p (change.m_new_state) || nonnull_p (change.m_new_state)))
// TODO: verify that it's the allocation stmt, not a copy
return label_text::borrow ("allocated here");
if (unchecked_p (change.m_old_state)
@@ -1179,6 +1179,21 @@ public:
{
return ev.formatted_print ("dereference of NULL %qE", ev.m_expr);
}
+
+ /* Implementation of pending_diagnostic::supercedes_p for
+ null-deref.
+
+ We want null-deref to supercede use-of-unitialized-value,
+ so that if we have these at the same stmt, we don't emit
+ a use-of-uninitialized, just the null-deref. */
+
+ bool supercedes_p (const pending_diagnostic &other) const final override
+ {
+ if (other.use_of_uninit_p ())
+ return true;
+
+ return false;
+ }
};
/* Concrete subclass for describing passing a NULL value to a
@@ -1915,12 +1930,20 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt,
return true;
}
- if (is_named_call_p (callee_fndecl, "operator new", call, 1))
- on_allocator_call (sm_ctxt, 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);
- else if (is_named_call_p (callee_fndecl, "operator delete", call, 1)
- || is_named_call_p (callee_fndecl, "operator delete", call, 2))
+ if (!is_placement_new_p (call))
+ {
+ bool returns_nonnull = !TREE_NOTHROW (callee_fndecl)
+ && flag_exceptions;
+ if (is_named_call_p (callee_fndecl, "operator new"))
+ on_allocator_call (sm_ctxt, call,
+ &m_scalar_delete, returns_nonnull);
+ else if (is_named_call_p (callee_fndecl, "operator new []"))
+ on_allocator_call (sm_ctxt, call,
+ &m_vector_delete, returns_nonnull);
+ }
+
+ if (is_named_call_p (callee_fndecl, "operator delete", call, 1)
+ || is_named_call_p (callee_fndecl, "operator delete", call, 2))
{
on_deallocator_call (sm_ctxt, node, call,
&m_scalar_delete.m_deallocator, 0);
@@ -970,6 +970,8 @@ poison_kind_to_str (enum poison_kind kind)
return "uninit";
case POISON_KIND_FREED:
return "freed";
+ case POISON_KIND_DELETED:
+ return "deleted";
case POISON_KIND_POPPED_STACK:
return "popped stack";
}
@@ -350,6 +350,9 @@ enum poison_kind
/* For use to describe freed memory. */
POISON_KIND_FREED,
+ /* For use to describe deleted memory. */
+ POISON_KIND_DELETED,
+
/* For use on pointers to regions within popped stack frames. */
POISON_KIND_POPPED_STACK
};
new file mode 100644
@@ -0,0 +1,70 @@
+// { dg-additional-options "-O0 -fno-analyzer-suppress-followups -fexceptions" }
+#include <new>
+
+struct A
+{
+ int x;
+ int y;
+};
+
+void test_spurious_null_warning_throwing ()
+{
+ int *x = new int; /* { dg-bogus "dereference of possibly-NULL" } */
+ int *y = new int (); /* { dg-bogus "dereference of possibly-NULL" "non-throwing" } */
+ int *arr = new int[3]; /* { dg-bogus "dereference of possibly-NULL" } */
+ A *a = new A (); /* { dg-bogus "dereference of possibly-NULL" "throwing new cannot be null" } */
+
+ int z = *y + 2;
+ z = *x + 4; /* { dg-bogus "dereference of possibly-NULL 'x'" } */
+ /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-* } .-1 } */
+ z = arr[0] + 4; /* { dg-bogus "dereference of possibly-NULL" } */
+ /* { dg-warning "use of uninitialized value '\\*arr'" "" { target *-*-* } .-1 } */
+
+ delete a;
+ delete y;
+ delete x;
+ delete[] arr;
+}
+
+void test_default_initialization ()
+{
+ int *y = ::new int;
+ int *x = ::new int (); /* { dg-bogus "dereference of possibly-NULL 'operator new" } */
+
+ int b = *x + 3; /* { dg-bogus "dereference of possibly-NULL" } */
+ /* { dg-bogus "use of uninitialized ‘*x’" "" { target *-*-* } .-1 } */
+ int a = *y + 2; /* { dg-bogus "dereference of possibly-NULL 'y'" } */
+ /* { dg-warning "use of uninitialized value '\\*y'" "no default init" { target *-*-* } .-1 } */
+
+ delete x;
+ delete y;
+}
+
+/* From clang core.uninitialized.NewArraySize
+new[] should not be called with an undefined size argument */
+
+void test_garbage_new_array ()
+{
+ int n;
+ int *arr = ::new int[n]; /* { dg-warning "use of uninitialized value 'n'" } */
+ arr[0] = 7;
+ ::delete[] arr; /* no warnings emitted here either */
+}
+
+void test_nonthrowing ()
+{
+ int* x = new(std::nothrow) int;
+ int* y = new(std::nothrow) int();
+ int* arr = new(std::nothrow) int[10];
+
+ int z = *y + 2; /* { dg-warning "dereference of NULL 'y'" } */
+ /* { dg-bogus "use of uninitialized value '\\*y'" "" { target *-*-* } .-1 } */
+ z = *x + 4; /* { dg-warning "dereference of possibly-NULL 'x'" } */
+ /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-* } .-1 } */
+ z = arr[0] + 4; /* { dg-warning "dereference of possibly-NULL 'arr'" } */
+ /* { dg-warning "use of uninitialized value '\\*arr'" "" { target *-*-* } .-1 } */
+
+ delete y;
+ delete x;
+ delete[] arr;
+}
new file mode 100644
@@ -0,0 +1,48 @@
+/* { dg-additional-options "-O0 -fno-exceptions -fno-analyzer-suppress-followups" } */
+#include <new>
+
+/* Test non-throwing variants of operator new */
+
+struct A
+{
+ int x;
+ int y;
+};
+
+void test_throwing ()
+{
+ int* x = new int;
+ int* y = new int(); /* { dg-warning "dereference of possibly-NULL" } */
+ int* arr = new int[10];
+ A *a = new A(); /* { dg-warning "dereference of possibly-NULL" } */
+
+ int z = *y + 2;
+ z = *x + 4; /* { dg-warning "dereference of possibly-NULL 'x'" } */
+ /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-* } .-1 } */
+ z = arr[0] + 4; /* { dg-warning "dereference of possibly-NULL 'arr'" } */
+ /* { dg-warning "use of uninitialized value '\\*arr'" "" { target *-*-* } .-1 } */
+ a->y = a->x + 3;
+
+ delete a;
+ delete y;
+ delete x;
+ delete[] arr;
+}
+
+void test_nonthrowing ()
+{
+ int* x = new(std::nothrow) int;
+ int* y = new(std::nothrow) int();
+ int* arr = new(std::nothrow) int[10];
+
+ int z = *y + 2; /* { dg-warning "dereference of NULL 'y'" } */
+ /* { dg-bogus "use of uninitialized value '\\*y'" "" { target *-*-* } .-1 } */
+ z = *x + 4; /* { dg-warning "dereference of possibly-NULL 'x'" } */
+ /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-* } .-1 } */
+ z = arr[0] + 4; /* { dg-warning "dereference of possibly-NULL 'arr'" } */
+ /* { dg-warning "use of uninitialized value '\\*arr'" "" { target *-*-* } .-1 } */
+
+ delete y;
+ delete x;
+ delete[] arr;
+}
@@ -14,6 +14,6 @@ struct int_and_addr {
int test (int_container ic)
{
- int_and_addr *iaddr = new (ic.addr ()) int_and_addr;
+ int_and_addr *iaddr = new (ic.addr ()) int_and_addr; /* { dg-warning "stack-based buffer overflow" } */
return iaddr->i;
}
new file mode 100644
@@ -0,0 +1,27 @@
+/* { dg-additional-options "-Wno-placement-new -Wno-analyzer-use-of-uninitialized-value" } */
+
+#include <new>
+#include <stdint.h>
+
+extern int get_buf_size ();
+
+void var_too_short ()
+{
+ int8_t s;
+ int64_t *lp = new (&s) int64_t; /* { dg-warning "stack-based buffer overflow" } */
+ /* { dg-warning "allocated buffer size is not a multiple of the pointee's size" "" { target *-*-* } .-1 } */
+}
+
+void static_buffer_too_short ()
+{
+ int n = 16;
+ int buf[n];
+ int *p = new (buf) int[n + 1]; /* { dg-warning "stack-based buffer overflow" } */
+}
+
+void symbolic_buffer_too_short ()
+{
+ int n = get_buf_size ();
+ char buf[n];
+ char *p = new (buf) char[n + 10]; /* { dg-warning "stack-based buffer overflow" } */
+}
@@ -14,15 +14,101 @@ void test_2 (void)
{
char buf[sizeof(int) * 10];
int *p = new(buf) int[10];
-}
+} // { dg-prune-output "-Wfree-nonheap-object" }
/* Delete of placement new. */
void test_3 (void)
{
char buf[sizeof(int)]; // { dg-message "region created on stack here" }
- int *p = new(buf) int (42);
+ int *p = new (buf) int (42);
delete p; // { dg-warning "memory on the stack" }
}
// { dg-prune-output "-Wfree-nonheap-object" }
+
+void test_4 (void)
+{
+ int buf[5]; // { dg-message "region created on stack here" }
+ int *p = new (&buf[2]) int (42);
+ delete p; // { dg-warning "memory on the stack" }
+}
+
+
+// { dg-prune-output "-Wfree-nonheap-object" }
+
+void test_write_placement_after_delete (void)
+{
+ short *s = ::new short;
+ short *lp = ::new (s) short;
+ ::delete s;
+ *lp = 12; /* { dg-warning "use after 'delete' of 'lp'" "write placement new after buffer deletion" } */
+}
+
+void test_read_placement_after_delete (void)
+{
+ short *s = ::new short;
+ short *lp = ::new (s) short;
+ ::delete s;
+ short m = *lp; // { dg-warning "use after 'delete' of 'lp'" "read placement new after buffer deletion" }
+}
+
+struct A
+{
+ int x;
+ int y;
+};
+
+void test_use_placement_after_destruction (void)
+{
+ A a;
+ int *lp = ::new (&a.y) int;
+ *lp = 2; /* { dg-bogus "-Wanalyzer-use-of-uninitialized-value" } */
+ a.~A();
+ int m = *lp; /* { dg-warning "use of uninitialized value '\\*lp'" "use of placement after the underlying buffer was destructed." } */
+}
+
+void test_initialization_through_placement (void)
+{
+ int x;
+ int *p = ::new (&x) int;
+ *p = 10;
+ int z = x + 2; /* { dg-bogus "use of uninitialized value 'x'" "x has been initialized through placement pointer" } */
+}
+
+void test_partial_initialization_through_placement (void)
+{
+ char buf[4];
+ char *p = ::new (&buf[2]) char;
+ *p = 10;
+ char *y = ::new (&buf[0]) char;
+ char z = buf[2] + 2; /* { dg-bogus "use of uninitialized value" } */
+ z = *y + 2; /* { dg-warning "use of uninitialized value '\\*y'" "y has only been partially initialized" } */
+}
+
+
+void test_delete_placement (void)
+{
+ A *a = ::new A; /* { dg-bogus "use of possibly-NULL 'operator new(8)' where non-null expected" "throwing new cannot be null" } */
+ int *z = ::new (&a->y) int;
+ a->~A(); // deconstruct properly
+ ::operator delete (a);
+ ::operator delete (z); /* { dg-warning "use after 'delete' of 'z'" } */
+}
+
+void test_delete_placement_2 (void)
+{
+ A *a = ::new A; /* { dg-bogus "use of possibly-NULL 'operator new(8)' where non-null expected" "throwing new cannot be null" } */
+ int *z = ::new (&a->y) int;
+ delete a;
+ ::operator delete (z); /* { dg-warning "use after 'delete' of 'z'" } */
+}
+
+void test_use_placement_after_deallocation (void)
+{
+ A *a = ::new A ();
+ int *lp = ::new (&a->y) int;
+ *lp = 2; /* { dg-bogus "use of uninitialized value" } */
+ ::operator delete (a);
+ int m = *lp; /* { dg-warning "use after 'delete' of 'lp'" "use of placement after the underlying buffer was deallocated." } */
+}