public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: priour.be@gmail.com
To: gcc-patches@gcc.gnu.org
Cc: dmalcolm@redhat.com, benjamin priour <vultkayn@gcc.gnu.org>
Subject: [WIP RFC v2] analyzer: Add support of placement new and improved operator new [PR105948]
Date: Wed, 16 Aug 2023 14:19:11 +0200	[thread overview]
Message-ID: <20230816121909.53047-1-vultkayn@gcc.gnu.org> (raw)
In-Reply-To: <eeaba9b8931680f2ecae895dc3018a3cc0155cfa.camel@redhat.com>

From: benjamin priour <vultkayn@gcc.gnu.org>

Hi,
(s/we/the analyzer/)

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 ?
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]

  <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;
  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
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".

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." } */
+}
-- 
2.34.1


  parent reply	other threads:[~2023-08-16 13:38 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-06 14:43 [PATCH] " priour.be
2023-07-21 20:10 ` David Malcolm
2023-07-31 11:46   ` Benjamin Priour
2023-07-31 22:53     ` David Malcolm
2023-08-16 12:19     ` priour.be [this message]
2023-08-16 22:34       ` [WIP RFC v2] " David Malcolm
2023-08-17 11:40         ` Benjamin Priour

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230816121909.53047-1-vultkayn@gcc.gnu.org \
    --to=priour.be@gmail.com \
    --cc=dmalcolm@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=vultkayn@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).