| Message ID | 20230816121909.53047-1-vultkayn@gcc.gnu.org |
|---|---|
| State | New |
| Headers | show |
| Series | [WIP,RFC,v2] analyzer: Add support of placement new and improved operator new [PR105948] | expand |
On Wed, 2023-08-16 at 14:19 +0200, priour.be@gmail.com wrote: > From: benjamin priour <vultkayn@gcc.gnu.org> > > Hi, > (s/we/the analyzer/) Hi Benjamin, thanks for the updated patch. > > I've been continuing my patch of supporting operator new variants > in the analyzer, and have added a few more test cases. > > > > > If "y" is null then the allocation failed and dereferencing > "y" will > > > cause > > > a segfault, not a "use-of-uninitialized-value". > > > Thus we should stick to 'dereference of NULL 'y'" only. > > > If "y" is non-null then the allocation succeeded and "*y" is > > > initialized > > > since we are calling a default initialization with the empty > > > parenthesis. > > > > I *think* it's possible to have the region_model have y > pointing to a > > heap_allocated_region of sizeof(int) size that's been > initialized, but > > still have the malloc state machine part of the program_state > say that > > the pointer is maybe-null. > > By maybe-null are you implying a new sm-malloc state ? Sorry, I was too vague here. I was referring to the "unchecked" state in sm-malloc.cc, which represents a pointer that's been returned from an allocator function, where the pointer hasn't yet been checked for being null/non-null. > I am not sure to follow on that front. > > > > > > > This led me to consider having "null-dereference" supersedes > > > "use-of-uninitialized-value", but > > > new PR 110830 made me reexamine it. > > > > > > I believe fixing PR 110830 is thus required before submitting > this > > > patch, > > > or we would have some extra irrelevant warnings. > > > > How bad would the problem be? PR 110830 looks a little > involved, so is > > there a way to get the current patch in without dragging that > extra > > complexity in? > > Having "null-dereference" supersedes "use-of-uninitialized-value" > would > cause false negative upon conditional return statement (similarly as > demonstrated > in PR 110830). > > Since PR 110830 is off for the moment, I have tried solving this > differently. > I have considered using known NULL constraints on > heap_allocated_region > as "initialized_value". > > You can see below in the diff of region_model::get_store_value > two versions of this approach. The version commented out proved to > solve > the issue of the spurious "use-of-unitialized-value" tagging along > calls to > "new(std::nothrow) ()". However, this version also shortcircuits the > diagnostics of the "null-dereference" warning. > > Given > /* { dg-additional-options "-O0 -fno-exceptions -fno-analyzer- > suppress-followups" } */ > #include <new> > > struct A > { > int x; > int y; > }; > > void test_nonthrowing () > { > A* y = new(std::nothrow) A(); > int z = y->x + 2; /* { dg-warning "dereference of NULL 'y'" } > */ > /* { dg-bogus "use of uninitialized value '\\*y'" "" { xfail *- > *-* } .-1 } */ > > delete y; > } > > The analyzer sees gimple > > <bb 2> : > _7 = operator new (8, ¬hrow); > if (_7 != 0B) > goto <bb 3>; [INV] > else > goto <bb 4>; [INV] I would have thought that at each branch of this conditional that region_model::add_constraint would be called, and within that we'd reach this code: 4339 /* Notify the context, if any. This exists so that the state machines 4340 in a program_state can be notified about the condition, and so can 4341 set sm-state for e.g. unchecked->checked, both for cfg-edges, and 4342 when synthesizing constraints as above. */ 4343 if (ctxt) 4344 ctxt->on_condition (lhs, op, rhs); This ought to call impl_region_model_context::on_condition in engine.cc, which ought to call malloc_state_machine::on_condition in sm-malloc.cc, and this ought to transition the sm-state of _7. Is something going wrong somewhere in the things I mentioned above? > > <bb 3> : > MEM[(struct A *)_7].x = 0; > MEM[(struct A *)_7].y = 0; > iftmp.0_11 = _7; > goto <bb 5>; [INV] > > <bb 4> : > iftmp.0_8 = _7; > > <bb 5> : > # iftmp.0_2 = PHI <iftmp.0_11(3), iftmp.0_8(4)> > y_12 = iftmp.0_2; > _1 = y_12->x; ...and at this point we have a deref from y_12, which on the path from bb 5 ought to be an svalue that has the "null" state in the sm-state machine, and thus malloc_state_machine::on_stmt ought to complain at _1 = y_12->x; here: 2094 else if (state == m_null) 2095 { 2096 tree diag_arg = sm_ctxt->get_diagnostic_tree 2097 sm_ctxt->warn (node, stmt, arg, 2098 make_unique<null_deref> (*this, diag_arg)); 2099 sm_ctxt->set_next_state (stmt, arg, m_stop); 2100 } That's what ought to be happening, and ought to give you the correct warning. > z_13 = _1 + 2; > y.1_14 = y_12; > if (y.1_14 != 0B) > goto <bb 6>; [INV] > else > goto <bb 7>; [INV] > > <bb 6> : > *y.1_14 ={v} {CLOBBER}; > operator delete (y.1_14, 8); > > The injurious path, causing the "use-of-uninit" warning is as > follows: > <bb 2> : > _7 = operator new (8, ¬hrow); > if (_7 != 0B) > ... > else <- Takes false branch > goto <bb 4>; [INV] > > ... > > <bb 4> : > iftmp.0_8 = _7; <- MEM[(struct A*) _7] is left uninit in this bb > > <bb 5> : > # iftmp.0_2 = PHI <iftmp.0_11(3), iftmp.0_8(4)> <- iftmp.0_2 = > iftmp.0_8(4) > y_12 = iftmp.0_2; > _1 = y_12->x; // deref of null y_12, use of uninit y_12->x > z_13 = _1 + 2; // check_for_poison sets _1 to unknown_svalue > y.1_14 = y_12; > if (y.1_14 != 0B) > goto <bb 6>; [INV] > else > goto <bb 7>; [INV] > > Then using the "commented-out" fix, iftmp.0_8 which had an uninit > value is Sorry, I'm not sure what you mean by the '"commented-out" fix' here. > forcibly set to constant_svalue(0), since the analyzer detects a NULL > constraint > on &heap_allocated_region. > Unfortunately, this loses all clusters binding on _7 and the > followings > variables, such as when we arrive at "_1 = y_12->x", we emit a > "null_deref" not because the heap_allocated_region is in a null > state, > but because we are dereferencing a constant "0". > Thus the analysis path no longer tracks down the creation of this > region, and the genese event is "iftmp.0_8 = _7". It sounds like you are getting a null_deref on the pointer, which sounds correct. If we've dereferenced a null pointer, we should probably terminate the path, which would stop the follow-up warnings about uninitialized fields; the initializations were skipped anyway (by going to bb 4, rather than bb 3). Does this help? Dave > > As you guess, this loss of information fails a lot of regression > tests, > although it achieves the goal of removing the "use-of-uninit" > warning. > > The second attempt (see get_store_value diff below, the non-commented > out block), actually does nothing, which as I understood through > debugging was to be expected. We are doing the same "constraints" > check > as the former version, but only as a last resort before resorting to > creating an initial or unknown svalue. > And instead of creating a constant_svalue(0) as before, now a NULL > constraint only prevents the creation of a poisoned_svalue(uninit) > by setting "check_poisoned" to false. > > However in > + if (reg->get_kind () == RK_FIELD || reg->get_kind () == > RK_ELEMENT) > + { > + const region *base_reg = reg->get_base_region (); > + const svalue *base_sval > + = m_store.get_any_binding (m_mgr->get_store_manager (), > base_reg); > + if (base_sval) > + { > + ... > + } > > base_sval is always NULL (which makes sense since otherwise we > wouldn't > need to create an initial_svalue). > > The spirit behind this second attempt is to not lose any sort of > information, therefore to not forcibly create constant_svalues. > > Thus, instead of assigning a null_svalue at "iftmp.0_8 = _7", > rather wait for the latest "_1 = y_12->x;" and prevent the creation > of a "poisoned_svalue (uninit)". > > The failings of the second attempt is to find the NULL constraint of > the heap_allocated_region (which still exists thanks to path > splitting) > given the COMPONENT_REF y_12->x; > > I am unsure as to how to do that, thus this RFC. > > > 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 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' nothrow variants are detected by checking the types > of the parameters. > Indeed, in a call to new (std::nothrow) (), although the chosen > overload > has signature 'operator new (void*, std::nothrow_t&)', where > parameter 1 > is a reference. In a placement new, parameter 1 will always be a > [void] > pointer. > > Prior to this patch, some buffers allocated with 'new', > then deleted and thereafter used would result in a > Wanalyzer-use-after-free warning. However, > the wording was "use after 'free' of" instead of the expected > "use after 'delete' of". > > This patch fixed this by introducing a new kind of > poisoned value. > > Signed-off-by: benjamin priour <vultkayn@gcc.gnu.org> > > gcc/analyzer/ChangeLog: > > * 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 > descendent 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 > parameters. > * region-model.cc (region_model::get_rvalue_1): > (region_model::get_store_value): Attempt to use a region > NULL constraint to emit a null_ptr_svalue instead of an > uninit > poisoned_svalue. > (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: > > * 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 | 111 +++++++++++++++- > -- > gcc/analyzer/region-model.cc | 68 ++++++++++- > gcc/analyzer/sm-malloc.cc | 17 ++- > 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 | 92 ++++++++++++++- > 13 files changed, 427 insertions(+), 26 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 > > diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h > index 93a28b4b5cf..f44e8296f5e 100644 > --- a/gcc/analyzer/analyzer.h > +++ b/gcc/analyzer/analyzer.h > @@ -401,6 +401,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); > > diff --git a/gcc/analyzer/call-details.cc b/gcc/analyzer/call- > details.cc > index fa86f55177a..0ac44f3726c 100644 > --- a/gcc/analyzer/call-details.cc > +++ b/gcc/analyzer/call-details.cc > @@ -283,6 +283,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 > diff --git a/gcc/analyzer/call-details.h b/gcc/analyzer/call- > details.h > index 0622cab7856..919338f88b9 100644 > --- a/gcc/analyzer/call-details.h > +++ b/gcc/analyzer/call-details.h > @@ -60,6 +60,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; > diff --git a/gcc/analyzer/kf-lang-cp.cc b/gcc/analyzer/kf-lang-cp.cc > index 393b4f25e79..cfb0584979b 100644 > --- a/gcc/analyzer/kf-lang-cp.cc > +++ b/gcc/analyzer/kf-lang-cp.cc > @@ -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,74 @@ 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 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 (), new_reg); > + = 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 +168,10 @@ 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 +181,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 > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region- > model.cc > index 494a9cdf149..a358794356e 100644 > --- a/gcc/analyzer/region-model.cc > +++ b/gcc/analyzer/region-model.cc > @@ -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", > @@ -2210,7 +2223,7 @@ region_model::get_rvalue_1 (path_var pv, > region_model_context *ctxt) const > case MEM_REF: > { > const region *ref_reg = get_lvalue (pv, ctxt); > - return get_store_value (ref_reg, ctxt); > + return get_store_value (ref_reg, ctxt); // INFO get store > value of field_region(cast_region(heap_allocated_region(19), ‘struct > A’), ‘int’, ‘int A::x’) > } > case OBJ_TYPE_REF: > { > @@ -2321,6 +2334,16 @@ region_model::get_store_value (const region > *reg, > = m_store.get_any_binding (m_mgr->get_store_manager (), reg); > if (sval) > { > +/* if (m_constraints->sval_constrained_p (sval)) > + { > + equiv_class_id sval_ecid = equiv_class_id::null (); > + if (m_constraints->get_equiv_class_by_svalue (sval, > &sval_ecid)) > + { > + const equiv_class& sval_ec = > sval_ecid.get_obj(*m_constraints); > + if (sval_ec.get_any_constant () != NULL_TREE) > + sval = sval_ec.m_cst_sval; > + } > + } */ > if (reg->get_type ()) > sval = m_mgr->get_or_create_cast (reg->get_type (), sval); > return sval; > @@ -2363,6 +2386,28 @@ region_model::get_store_value (const region > *reg, > == RK_GLOBALS) > return get_initial_value_for_global (reg); > > + /* If there exists a NULL constraint on the parent region, > + then do not create a poisoned_uninit value. */ > + if (reg->get_kind () == RK_FIELD || reg->get_kind () == > RK_ELEMENT) > + { > + const region *base_reg = reg->get_base_region (); > + const svalue *base_sval > + = m_store.get_any_binding (m_mgr->get_store_manager (), > base_reg); > + if (base_sval) > + { > + if (m_constraints->sval_constrained_p (base_sval)) > + { > + equiv_class_id sval_ecid = equiv_class_id::null (); > + if (m_constraints->get_equiv_class_by_svalue > (base_sval, &sval_ecid)) > + { > + const equiv_class& sval_ec = > sval_ecid.get_obj(*m_constraints); > + if (const_tree cst = sval_ec.get_any_constant ()) > + if (::zerop (cst)) > + check_poisoned = false; > + } > + } > + } > + } > return m_mgr->get_or_create_initial_value (reg, check_poisoned); > } > > @@ -3582,6 +3627,27 @@ 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). */ > diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc > index ec763254b29..567237cf1ae 100644 > --- a/gcc/analyzer/sm-malloc.cc > +++ b/gcc/analyzer/sm-malloc.cc > @@ -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) > @@ -1915,11 +1915,16 @@ 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) > + 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, > diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc > index 064627f3dcc..35eb8307b20 100644 > --- a/gcc/analyzer/svalue.cc > +++ b/gcc/analyzer/svalue.cc > @@ -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"; > } > diff --git a/gcc/analyzer/svalue.h b/gcc/analyzer/svalue.h > index 5492b1e0b7c..263a0d7af6f 100644 > --- a/gcc/analyzer/svalue.h > +++ b/gcc/analyzer/svalue.h > @@ -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 > }; > diff --git a/gcc/testsuite/g++.dg/analyzer/new-2.C > b/gcc/testsuite/g++.dg/analyzer/new-2.C > new file mode 100644 > index 00000000000..9e0315b3f44 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/analyzer/new-2.C > @@ -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'" "" { xfail *-*-* > } .-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; > +} > diff --git a/gcc/testsuite/g++.dg/analyzer/noexcept-new.C > b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C > new file mode 100644 > index 00000000000..4a5f526b0f3 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C > @@ -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'" "" { xfail *-*-* > } .-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; > +} > diff --git a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement- > new.C b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C > index d3076c3cf25..33872e6ddab 100644 > --- a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C > +++ b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C > @@ -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; > } > diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new-size.C > b/gcc/testsuite/g++.dg/analyzer/placement-new-size.C > new file mode 100644 > index 00000000000..4536d361b4a > --- /dev/null > +++ b/gcc/testsuite/g++.dg/analyzer/placement-new-size.C > @@ -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" } */ > +} > diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new.C > b/gcc/testsuite/g++.dg/analyzer/placement-new.C > index 89055491a48..8edd778131b 100644 > --- a/gcc/testsuite/g++.dg/analyzer/placement-new.C > +++ b/gcc/testsuite/g++.dg/analyzer/placement-new.C > @@ -1,3 +1,5 @@ > +/* { dg-additional-options "-Wno-free-nonheap-object -fexceptions" } > */ > + > #include <new> > > /* Placement new. */ > @@ -14,15 +16,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." } */ > +}
On Thu, Aug 17, 2023 at 12:34 AM David Malcolm <dmalcolm@redhat.com> wrote: > On Wed, 2023-08-16 at 14:19 +0200, priour.be@gmail.com wrote: > > From: benjamin priour <vultkayn@gcc.gnu.org> > > > > Hi, > > (s/we/the analyzer/) > > Hi Benjamin, thanks for the updated patch. > > > > > I've been continuing my patch of supporting operator new variants > > in the analyzer, and have added a few more test cases. > > > > > > > > If "y" is null then the allocation failed and dereferencing > > "y" will > > > > cause > > > > a segfault, not a "use-of-uninitialized-value". > > > > Thus we should stick to 'dereference of NULL 'y'" only. > > > > If "y" is non-null then the allocation succeeded and "*y" is > > > > initialized > > > > since we are calling a default initialization with the empty > > > > parenthesis. > > > > > > I *think* it's possible to have the region_model have y > > pointing to a > > > heap_allocated_region of sizeof(int) size that's been > > initialized, but > > > still have the malloc state machine part of the program_state > > say that > > > the pointer is maybe-null. > > > > By maybe-null are you implying a new sm-malloc state ? > > Sorry, I was too vague here. > > I was referring to the "unchecked" state in sm-malloc.cc, which > represents a pointer that's been returned from an allocator function, > where the pointer hasn't yet been checked for being null/non-null. > > Oh I see then. Unfortunately I don't think initializing the heap_allocated_region while having he unchecked state is doable here. I could do it in the kf_operator_new::on_call_{pre,post} hook, but it's not actually operator new that initiliaze the allocated region. For calls such as "A a = new (nothrow) A;", then 'a' is actually never initialized, therefore we need the heap_allocated_region to reflect that. > > I am not sure to follow on that front. > > > > > > > > > > > This led me to consider having "null-dereference" supersedes > > > > "use-of-uninitialized-value", but > > > > new PR 110830 made me reexamine it. > > > > > > > > I believe fixing PR 110830 is thus required before submitting > > this > > > > patch, > > > > or we would have some extra irrelevant warnings. > > > > > > How bad would the problem be? PR 110830 looks a little > > involved, so is > > > there a way to get the current patch in without dragging that > > extra > > > complexity in? > > > > Having "null-dereference" supersedes "use-of-uninitialized-value" > > would > > cause false negative upon conditional return statement (similarly as > > demonstrated > > in PR 110830). > > > > Since PR 110830 is off for the moment, I have tried solving this > > differently. > > I have considered using known NULL constraints on > > heap_allocated_region > > as "initialized_value". > > > > You can see below in the diff of region_model::get_store_value > > two versions of this approach. The version commented out proved to > > solve > > the issue of the spurious "use-of-unitialized-value" tagging along > > calls to > > "new(std::nothrow) ()". However, this version also shortcircuits the > > diagnostics of the "null-dereference" warning. > > > > Given > > /* { dg-additional-options "-O0 -fno-exceptions -fno-analyzer- > > suppress-followups" } */ > > #include <new> > > > > struct A > > { > > int x; > > int y; > > }; > > > > void test_nonthrowing () > > { > > A* y = new(std::nothrow) A(); > > int z = y->x + 2; /* { dg-warning "dereference of NULL 'y'" } > > */ > > /* { dg-bogus "use of uninitialized value '\\*y'" "" { xfail *- > > *-* } .-1 } */ > > > > delete y; > > } > > > > The analyzer sees gimple > > > > <bb 2> : > > _7 = operator new (8, ¬hrow); > > if (_7 != 0B) > > goto <bb 3>; [INV] > > else > > goto <bb 4>; [INV] > > I would have thought that at each branch of this conditional that > region_model::add_constraint would be called, and within that we'd > reach this code: > > 4339 /* Notify the context, if any. This exists so that the state > machines > 4340 in a program_state can be notified about the condition, and > so can > 4341 set sm-state for e.g. unchecked->checked, both for cfg-edges, > and > 4342 when synthesizing constraints as above. */ > 4343 if (ctxt) > 4344 ctxt->on_condition (lhs, op, rhs); > > This ought to call impl_region_model_context::on_condition in > engine.cc, which ought to call malloc_state_machine::on_condition in > sm-malloc.cc, and this ought to transition the sm-state of _7. > > Is something going wrong somewhere in the things I mentioned above? > > Nope. Everything's happening as you say. > > > > <bb 3> : > > MEM[(struct A *)_7].x = 0; > > MEM[(struct A *)_7].y = 0; > > iftmp.0_11 = _7; > > goto <bb 5>; [INV] > > > > <bb 4> : > > iftmp.0_8 = _7; > > > > <bb 5> : > > # iftmp.0_2 = PHI <iftmp.0_11(3), iftmp.0_8(4)> > > y_12 = iftmp.0_2; > > _1 = y_12->x; > > ...and at this point we have a deref from y_12, which on the path from > bb 5 ought to be an svalue that has the "null" state in the sm-state > machine, and thus malloc_state_machine::on_stmt ought to complain at > _1 = y_12->x; > here: > > 2094 else if (state == m_null) > 2095 { > 2096 tree diag_arg = sm_ctxt->get_diagnostic_tree > 2097 sm_ctxt->warn (node, stmt, arg, > 2098 make_unique<null_deref> (*this, > diag_arg)); > 2099 sm_ctxt->set_next_state (stmt, arg, m_stop); > 2100 } > > That's what ought to be happening, and ought to give you the correct > warning. > > > (nods) > > > z_13 = _1 + 2; > > y.1_14 = y_12; > > if (y.1_14 != 0B) > > goto <bb 6>; [INV] > > else > > goto <bb 7>; [INV] > > > > <bb 6> : > > *y.1_14 ={v} {CLOBBER}; > > operator delete (y.1_14, 8); > > > > The injurious path, causing the "use-of-uninit" warning is as > > follows: > > <bb 2> : > > _7 = operator new (8, ¬hrow); > > if (_7 != 0B) > > ... > > else <- Takes false branch > > goto <bb 4>; [INV] > > > > ... > > > > <bb 4> : > > iftmp.0_8 = _7; <- MEM[(struct A*) _7] is left uninit in this bb > > > > <bb 5> : > > # iftmp.0_2 = PHI <iftmp.0_11(3), iftmp.0_8(4)> <- iftmp.0_2 = > > iftmp.0_8(4) > > y_12 = iftmp.0_2; > > _1 = y_12->x; // deref of null y_12, use of uninit y_12->x > > z_13 = _1 + 2; // check_for_poison sets _1 to unknown_svalue > > y.1_14 = y_12; > > if (y.1_14 != 0B) > > goto <bb 6>; [INV] > > else > > goto <bb 7>; [INV] > > > > Then using the "commented-out" fix, iftmp.0_8 which had an uninit > > value is > > Sorry, I'm not sure what you mean by the '"commented-out" fix' here. > > First attempt (I agree the comment block wasn't the most noticeable thing amidst the diff). Check for constraints when the region was found with bindings. @@ -2321,6 +2334,16 @@ region_model::get_store_value (const region *reg, = m_store.get_any_binding (m_mgr->get_store_manager (), reg); if (sval) { + if (m_constraints->sval_constrained_p (sval)) + { + equiv_class_id sval_ecid = equiv_class_id::null (); + if (m_constraints->get_equiv_class_by_svalue (sval, &sval_ecid)) + { + const equiv_class& sval_ec = sval_ecid.get_obj(*m_constraints); + if (sval_ec.get_any_constant () != NULL_TREE) + sval = sval_ec.m_cst_sval; + } + } if (reg->get_type ()) sval = m_mgr->get_or_create_cast (reg->get_type (), sval); return sval; Second attempt in get_store_value, check for constraints right before the final return statement. @@ -2363,6 +2386,28 @@ region_model::get_store_value (const region *reg, == RK_GLOBALS) return get_initial_value_for_global (reg); + /* If there exists a NULL constraint on the parent region, + then do not create a poisoned_uninit value. */ + if (reg->get_kind () == RK_FIELD || reg->get_kind () == RK_ELEMENT) + { + const region *base_reg = reg->get_base_region (); + const svalue *base_sval + = m_store.get_any_binding (m_mgr->get_store_manager (), base_reg); + if (base_sval) + { + if (m_constraints->sval_constrained_p (base_sval)) + { + equiv_class_id sval_ecid = equiv_class_id::null (); + if (m_constraints->get_equiv_class_by_svalue (base_sval, &sval_ecid)) + { + const equiv_class& sval_ec = sval_ecid.get_obj(*m_constraints); + if (const_tree cst = sval_ec.get_any_constant ()) + if (::zerop (cst)) + check_poisoned = false; + } + } + } + } return m_mgr->get_or_create_initial_value (reg, check_poisoned); } > forcibly set to constant_svalue(0), since the analyzer detects a NULL > > constraint > > on &heap_allocated_region. > > Unfortunately, this loses all clusters binding on _7 and the > > followings > > variables, such as when we arrive at "_1 = y_12->x", we emit a > > "null_deref" not because the heap_allocated_region is in a null > > state, > > but because we are dereferencing a constant "0". > > Thus the analysis path no longer tracks down the creation of this > > region, and the genese event is "iftmp.0_8 = _7". > > It sounds like you are getting a null_deref on the pointer, which > sounds correct. > > If we've dereferenced a null pointer, we should probably terminate the > path, which would stop the follow-up warnings about uninitialized > fields; the initializations were skipped anyway (by going to bb 4, > rather than bb 3). > > We both agree on that. The issue is that the (first) poisoned diagnostic is emitted during the processing of region_model::on_stmt_pre, into region_model::on_assignment, into region_model::get_gassign_result , whereas the null_deref is emitted by sm_malloc::on_stmt. exploded_node::on_stmt | exploded_node::on_stmt_pre | region_model::on_stmt_pre | region_model:::on assignment | region_model::get_gassign_result -> calls get_rvalue that emits poisoned_value diagnostic | sm-malloc.on_stmt [/* Handle dereference, state == m_null */] -> emits null_deref diagnostic | terminate_path The null_deref diagnostic does actually terminate the path, but too late to prevent the first poisoned_diagnostic, already saved. That's why I'm trying to get information out of the constraints, that *are* correctly applied by the prior on_condition, so that I can nip the poisoned_diagnostic without relying on sm-malloc, which happens too late. Thanks, Benjamin. > Does this help? > Dave > > > > > As you guess, this loss of information fails a lot of regression > > tests, > > although it achieves the goal of removing the "use-of-uninit" > > warning. > > > > The second attempt (see get_store_value diff below, the non-commented > > out block), actually does nothing, which as I understood through > > debugging was to be expected. We are doing the same "constraints" > > check > > as the former version, but only as a last resort before resorting to > > creating an initial or unknown svalue. > > And instead of creating a constant_svalue(0) as before, now a NULL > > constraint only prevents the creation of a poisoned_svalue(uninit) > > by setting "check_poisoned" to false. > > > > However in > > + if (reg->get_kind () == RK_FIELD || reg->get_kind () == > > RK_ELEMENT) > > + { > > + const region *base_reg = reg->get_base_region (); > > + const svalue *base_sval > > + = m_store.get_any_binding (m_mgr->get_store_manager (), > > base_reg); > > + if (base_sval) > > + { > > + ... > > + } > > > > base_sval is always NULL (which makes sense since otherwise we > > wouldn't > > need to create an initial_svalue). > > > > The spirit behind this second attempt is to not lose any sort of > > information, therefore to not forcibly create constant_svalues. > > > > Thus, instead of assigning a null_svalue at "iftmp.0_8 = _7", > > rather wait for the latest "_1 = y_12->x;" and prevent the creation > > of a "poisoned_svalue (uninit)". > > > > The failings of the second attempt is to find the NULL constraint of > > the heap_allocated_region (which still exists thanks to path > > splitting) > > given the COMPONENT_REF y_12->x; > > > > I am unsure as to how to do that, thus this RFC. > > > > > > 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 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' nothrow variants are detected by checking the types > > of the parameters. > > Indeed, in a call to new (std::nothrow) (), although the chosen > > overload > > has signature 'operator new (void*, std::nothrow_t&)', where > > parameter 1 > > is a reference. In a placement new, parameter 1 will always be a > > [void] > > pointer. > > > > Prior to this patch, some buffers allocated with 'new', > > then deleted and thereafter used would result in a > > Wanalyzer-use-after-free warning. However, > > the wording was "use after 'free' of" instead of the expected > > "use after 'delete' of". > > > > This patch fixed this by introducing a new kind of > > poisoned value. > > > > Signed-off-by: benjamin priour <vultkayn@gcc.gnu.org> > > > > gcc/analyzer/ChangeLog: > > > > * 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 > > descendent 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 > > parameters. > > * region-model.cc (region_model::get_rvalue_1): > > (region_model::get_store_value): Attempt to use a region > > NULL constraint to emit a null_ptr_svalue instead of an > > uninit > > poisoned_svalue. > > (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: > > > > * 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 | 111 +++++++++++++++- > > -- > > gcc/analyzer/region-model.cc | 68 ++++++++++- > > gcc/analyzer/sm-malloc.cc | 17 ++- > > 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 | 92 ++++++++++++++- > > 13 files changed, 427 insertions(+), 26 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 > > > > diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h > > index 93a28b4b5cf..f44e8296f5e 100644 > > --- a/gcc/analyzer/analyzer.h > > +++ b/gcc/analyzer/analyzer.h > > @@ -401,6 +401,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); > > > > diff --git a/gcc/analyzer/call-details.cc b/gcc/analyzer/call- > > details.cc > > index fa86f55177a..0ac44f3726c 100644 > > --- a/gcc/analyzer/call-details.cc > > +++ b/gcc/analyzer/call-details.cc > > @@ -283,6 +283,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 > > diff --git a/gcc/analyzer/call-details.h b/gcc/analyzer/call- > > details.h > > index 0622cab7856..919338f88b9 100644 > > --- a/gcc/analyzer/call-details.h > > +++ b/gcc/analyzer/call-details.h > > @@ -60,6 +60,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; > > diff --git a/gcc/analyzer/kf-lang-cp.cc b/gcc/analyzer/kf-lang-cp.cc > > index 393b4f25e79..cfb0584979b 100644 > > --- a/gcc/analyzer/kf-lang-cp.cc > > +++ b/gcc/analyzer/kf-lang-cp.cc > > @@ -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,74 @@ 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 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 (), new_reg); > > + = 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 +168,10 @@ 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 +181,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 > > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region- > > model.cc > > index 494a9cdf149..a358794356e 100644 > > --- a/gcc/analyzer/region-model.cc > > +++ b/gcc/analyzer/region-model.cc > > @@ -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", > > @@ -2210,7 +2223,7 @@ region_model::get_rvalue_1 (path_var pv, > > region_model_context *ctxt) const > > case MEM_REF: > > { > > const region *ref_reg = get_lvalue (pv, ctxt); > > - return get_store_value (ref_reg, ctxt); > > + return get_store_value (ref_reg, ctxt); // INFO get store > > value of field_region(cast_region(heap_allocated_region(19), ‘struct > > A’), ‘int’, ‘int A::x’) > > } > > case OBJ_TYPE_REF: > > { > > @@ -2321,6 +2334,16 @@ region_model::get_store_value (const region > > *reg, > > = m_store.get_any_binding (m_mgr->get_store_manager (), reg); > > if (sval) > > { > > +/* if (m_constraints->sval_constrained_p (sval)) > > + { > > + equiv_class_id sval_ecid = equiv_class_id::null (); > > + if (m_constraints->get_equiv_class_by_svalue (sval, > > &sval_ecid)) > > + { > > + const equiv_class& sval_ec = > > sval_ecid.get_obj(*m_constraints); > > + if (sval_ec.get_any_constant () != NULL_TREE) > > + sval = sval_ec.m_cst_sval; > > + } > > + } */ > > if (reg->get_type ()) > > sval = m_mgr->get_or_create_cast (reg->get_type (), sval); > > return sval; > > @@ -2363,6 +2386,28 @@ region_model::get_store_value (const region > > *reg, > > == RK_GLOBALS) > > return get_initial_value_for_global (reg); > > > > + /* If there exists a NULL constraint on the parent region, > > + then do not create a poisoned_uninit value. */ > > + if (reg->get_kind () == RK_FIELD || reg->get_kind () == > > RK_ELEMENT) > > + { > > + const region *base_reg = reg->get_base_region (); > > + const svalue *base_sval > > + = m_store.get_any_binding (m_mgr->get_store_manager (), > > base_reg); > > + if (base_sval) > > + { > > + if (m_constraints->sval_constrained_p (base_sval)) > > + { > > + equiv_class_id sval_ecid = equiv_class_id::null (); > > + if (m_constraints->get_equiv_class_by_svalue > > (base_sval, &sval_ecid)) > > + { > > + const equiv_class& sval_ec = > > sval_ecid.get_obj(*m_constraints); > > + if (const_tree cst = sval_ec.get_any_constant ()) > > + if (::zerop (cst)) > > + check_poisoned = false; > > + } > > + } > > + } > > + } > > return m_mgr->get_or_create_initial_value (reg, check_poisoned); > > } > > > > @@ -3582,6 +3627,27 @@ 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). */ > > diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc > > index ec763254b29..567237cf1ae 100644 > > --- a/gcc/analyzer/sm-malloc.cc > > +++ b/gcc/analyzer/sm-malloc.cc > > @@ -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) > > @@ -1915,11 +1915,16 @@ 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) > > + 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, > > diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc > > index 064627f3dcc..35eb8307b20 100644 > > --- a/gcc/analyzer/svalue.cc > > +++ b/gcc/analyzer/svalue.cc > > @@ -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"; > > } > > diff --git a/gcc/analyzer/svalue.h b/gcc/analyzer/svalue.h > > index 5492b1e0b7c..263a0d7af6f 100644 > > --- a/gcc/analyzer/svalue.h > > +++ b/gcc/analyzer/svalue.h > > @@ -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 > > }; > > diff --git a/gcc/testsuite/g++.dg/analyzer/new-2.C > > b/gcc/testsuite/g++.dg/analyzer/new-2.C > > new file mode 100644 > > index 00000000000..9e0315b3f44 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/analyzer/new-2.C > > @@ -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'" "" { xfail *-*-* > > } .-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; > > +} > > diff --git a/gcc/testsuite/g++.dg/analyzer/noexcept-new.C > > b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C > > new file mode 100644 > > index 00000000000..4a5f526b0f3 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C > > @@ -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'" "" { xfail *-*-* > > } .-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; > > +} > > diff --git a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement- > > new.C b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C > > index d3076c3cf25..33872e6ddab 100644 > > --- a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C > > +++ b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C > > @@ -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; > > } > > diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new-size.C > > b/gcc/testsuite/g++.dg/analyzer/placement-new-size.C > > new file mode 100644 > > index 00000000000..4536d361b4a > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/analyzer/placement-new-size.C > > @@ -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" } */ > > +} > > diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new.C > > b/gcc/testsuite/g++.dg/analyzer/placement-new.C > > index 89055491a48..8edd778131b 100644 > > --- a/gcc/testsuite/g++.dg/analyzer/placement-new.C > > +++ b/gcc/testsuite/g++.dg/analyzer/placement-new.C > > @@ -1,3 +1,5 @@ > > +/* { dg-additional-options "-Wno-free-nonheap-object -fexceptions" } > > */ > > + > > #include <new> > > > > /* Placement new. */ > > @@ -14,15 +16,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." } */ > > +} > >
diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h index 93a28b4b5cf..f44e8296f5e 100644 --- a/gcc/analyzer/analyzer.h +++ b/gcc/analyzer/analyzer.h @@ -401,6 +401,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); diff --git a/gcc/analyzer/call-details.cc b/gcc/analyzer/call-details.cc index fa86f55177a..0ac44f3726c 100644 --- a/gcc/analyzer/call-details.cc +++ b/gcc/analyzer/call-details.cc @@ -283,6 +283,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 diff --git a/gcc/analyzer/call-details.h b/gcc/analyzer/call-details.h index 0622cab7856..919338f88b9 100644 --- a/gcc/analyzer/call-details.h +++ b/gcc/analyzer/call-details.h @@ -60,6 +60,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; diff --git a/gcc/analyzer/kf-lang-cp.cc b/gcc/analyzer/kf-lang-cp.cc index 393b4f25e79..cfb0584979b 100644 --- a/gcc/analyzer/kf-lang-cp.cc +++ b/gcc/analyzer/kf-lang-cp.cc @@ -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,74 @@ 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 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 (), new_reg); + = 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 +168,10 @@ 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 +181,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 diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 494a9cdf149..a358794356e 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -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", @@ -2210,7 +2223,7 @@ region_model::get_rvalue_1 (path_var pv, region_model_context *ctxt) const case MEM_REF: { const region *ref_reg = get_lvalue (pv, ctxt); - return get_store_value (ref_reg, ctxt); + return get_store_value (ref_reg, ctxt); // INFO get store value of field_region(cast_region(heap_allocated_region(19), ‘struct A’), ‘int’, ‘int A::x’) } case OBJ_TYPE_REF: { @@ -2321,6 +2334,16 @@ region_model::get_store_value (const region *reg, = m_store.get_any_binding (m_mgr->get_store_manager (), reg); if (sval) { +/* if (m_constraints->sval_constrained_p (sval)) + { + equiv_class_id sval_ecid = equiv_class_id::null (); + if (m_constraints->get_equiv_class_by_svalue (sval, &sval_ecid)) + { + const equiv_class& sval_ec = sval_ecid.get_obj(*m_constraints); + if (sval_ec.get_any_constant () != NULL_TREE) + sval = sval_ec.m_cst_sval; + } + } */ if (reg->get_type ()) sval = m_mgr->get_or_create_cast (reg->get_type (), sval); return sval; @@ -2363,6 +2386,28 @@ region_model::get_store_value (const region *reg, == RK_GLOBALS) return get_initial_value_for_global (reg); + /* If there exists a NULL constraint on the parent region, + then do not create a poisoned_uninit value. */ + if (reg->get_kind () == RK_FIELD || reg->get_kind () == RK_ELEMENT) + { + const region *base_reg = reg->get_base_region (); + const svalue *base_sval + = m_store.get_any_binding (m_mgr->get_store_manager (), base_reg); + if (base_sval) + { + if (m_constraints->sval_constrained_p (base_sval)) + { + equiv_class_id sval_ecid = equiv_class_id::null (); + if (m_constraints->get_equiv_class_by_svalue (base_sval, &sval_ecid)) + { + const equiv_class& sval_ec = sval_ecid.get_obj(*m_constraints); + if (const_tree cst = sval_ec.get_any_constant ()) + if (::zerop (cst)) + check_poisoned = false; + } + } + } + } return m_mgr->get_or_create_initial_value (reg, check_poisoned); } @@ -3582,6 +3627,27 @@ 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). */ diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc index ec763254b29..567237cf1ae 100644 --- a/gcc/analyzer/sm-malloc.cc +++ b/gcc/analyzer/sm-malloc.cc @@ -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) @@ -1915,11 +1915,16 @@ 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) + 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, diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc index 064627f3dcc..35eb8307b20 100644 --- a/gcc/analyzer/svalue.cc +++ b/gcc/analyzer/svalue.cc @@ -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"; } diff --git a/gcc/analyzer/svalue.h b/gcc/analyzer/svalue.h index 5492b1e0b7c..263a0d7af6f 100644 --- a/gcc/analyzer/svalue.h +++ b/gcc/analyzer/svalue.h @@ -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 }; diff --git a/gcc/testsuite/g++.dg/analyzer/new-2.C b/gcc/testsuite/g++.dg/analyzer/new-2.C new file mode 100644 index 00000000000..9e0315b3f44 --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/new-2.C @@ -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'" "" { xfail *-*-* } .-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; +} diff --git a/gcc/testsuite/g++.dg/analyzer/noexcept-new.C b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C new file mode 100644 index 00000000000..4a5f526b0f3 --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C @@ -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'" "" { xfail *-*-* } .-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; +} diff --git a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C index d3076c3cf25..33872e6ddab 100644 --- a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C +++ b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C @@ -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; } diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new-size.C b/gcc/testsuite/g++.dg/analyzer/placement-new-size.C new file mode 100644 index 00000000000..4536d361b4a --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/placement-new-size.C @@ -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" } */ +} diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new.C b/gcc/testsuite/g++.dg/analyzer/placement-new.C index 89055491a48..8edd778131b 100644 --- a/gcc/testsuite/g++.dg/analyzer/placement-new.C +++ b/gcc/testsuite/g++.dg/analyzer/placement-new.C @@ -1,3 +1,5 @@ +/* { dg-additional-options "-Wno-free-nonheap-object -fexceptions" } */ + #include <new> /* Placement new. */ @@ -14,15 +16,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." } */ +}