On Wed, 2023-08-16 at 14:19 +0200, priour...@gmail.com wrote:
> From: benjamin priour <vultk...@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, &nothrow);
>   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, &nothrow);
>   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 <vultk...@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." } */
> +}

Reply via email to