diff mbox series

[v2] c++: Fix another crash with invalid new operators [PR117463]

Message ID 010201931b9ba887-d833ec77-35ef-44c3-99aa-6dcbbd36050b-000000@eu-west-1.amazonses.com
State New
Headers show
Series [v2] c++: Fix another crash with invalid new operators [PR117463] | expand

Commit Message

Simon Martin Nov. 11, 2024, 2:23 p.m. UTC 
Hi Jason,

On 6 Nov 2024, at 20:47, Jason Merrill wrote:

> On 11/6/24 2:23 PM, Simon Martin wrote:
>> Even though this PR is very close to PR117101, it's not addressed by 
>> the
>> fix I made through r15-4958-g5821f5c8c89a05 because 
>> cxx_placement_new_fn
>> has the very same issue as std_placement_new_fn_p used to have.
>>
>> This patch fixes the issue exactly the same, by checking the first
>> parameter against NULL_TREE. I considered somehow sharing code 
>> between
>> the two *_placement_new_fn* static functions, but it looked pointless
>> since the shared part ("the second parameter is the last one, and a
>> pointer") is unlikely to be useful elsewhere.
>
> It seems those two functions are intended to test the exact same 
> thing: whether the argument is ::operator new(size_t, void*).
>
> I think cxx_placement_new_fn should just be
>
> return (cxx_dialect >= cxx20 && std_placement_new_fn_p (fndecl));
>
> and some of the tests in cxx_placement_new_fn move into 
> std_placement_new_fn_p to accommodate callers that e.g. don't already 
> know we're dealing with operator new.
Thanks for the suggestion, makes sense.

I did not realise that some of the tests in those functions were 
actually redundant (like the tree code or global namespace ones, implied 
by DECL_NAMESPACE_SCOPE_P and IDENTIFIER_NEW_OP_P) and made them look 
more different than they really are.

The attached patch has been successfully tested on x86_64-pc-linux-gnu. 
OK for trunk?

Thanks, Simon
From 65c73e12cb25ccfa8dd06fa8d765d993f973aecf Mon Sep 17 00:00:00 2001
From: Simon Martin <simon@nasilyan.com>
Date: Wed, 6 Nov 2024 15:39:23 +0100
Subject: [PATCH] c++: Fix another crash with invalid new operators [PR117463]

Even though this PR is very close to PR117101, it's not addressed by the
fix I made through r15-4958-g5821f5c8c89a05 because cxx_placement_new_fn
has the very same issue as std_placement_new_fn_p used to have.

As suggested by Jason, this patch changes both functions so that
cxx_placement_new_fn leverages std_placement_new_fn_p which reduces code
duplication and fixes the PR.

Successfully tested on x86_64-pc-linux-gnu.

	PR c++/117463

gcc/cp/ChangeLog:

	* constexpr.cc (cxx_placement_new_fn): Implement in terms of
	std_placement_new_fn_p.
	* cp-tree.h (std_placement_new_fn_p): Declare.
	* init.cc (std_placement_new_fn_p): Add missing checks to ensure
	that fndecl is a non-replaceable ::operator new.

gcc/testsuite/ChangeLog:

	* g++.dg/init/new54.C: New test.

---
 gcc/cp/constexpr.cc               | 13 +------------
 gcc/cp/cp-tree.h                  |  1 +
 gcc/cp/init.cc                    |  6 ++++--
 gcc/testsuite/g++.dg/init/new54.C | 14 ++++++++++++++
 4 files changed, 20 insertions(+), 14 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/init/new54.C

Comments

Jason Merrill Nov. 11, 2024, 6:41 p.m. UTC | #1 
On 11/11/24 9:23 AM, Simon Martin wrote:
> Hi Jason,
> 
> On 6 Nov 2024, at 20:47, Jason Merrill wrote:
> 
>> On 11/6/24 2:23 PM, Simon Martin wrote:
>>> Even though this PR is very close to PR117101, it's not addressed by
>>> the
>>> fix I made through r15-4958-g5821f5c8c89a05 because
>>> cxx_placement_new_fn
>>> has the very same issue as std_placement_new_fn_p used to have.
>>>
>>> This patch fixes the issue exactly the same, by checking the first
>>> parameter against NULL_TREE. I considered somehow sharing code
>>> between
>>> the two *_placement_new_fn* static functions, but it looked pointless
>>> since the shared part ("the second parameter is the last one, and a
>>> pointer") is unlikely to be useful elsewhere.
>>
>> It seems those two functions are intended to test the exact same
>> thing: whether the argument is ::operator new(size_t, void*).
>>
>> I think cxx_placement_new_fn should just be
>>
>> return (cxx_dialect >= cxx20 && std_placement_new_fn_p (fndecl));
>>
>> and some of the tests in cxx_placement_new_fn move into
>> std_placement_new_fn_p to accommodate callers that e.g. don't already
>> know we're dealing with operator new.
> Thanks for the suggestion, makes sense.
> 
> I did not realise that some of the tests in those functions were
> actually redundant (like the tree code or global namespace ones, implied
> by DECL_NAMESPACE_SCOPE_P and IDENTIFIER_NEW_OP_P) and made them look
> more different than they really are.
> 
> The attached patch has been successfully tested on x86_64-pc-linux-gnu.
> OK for trunk?

OK.
diff mbox series

Patch

diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index 71e6dc4ef32..c097860e655 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -2327,18 +2327,7 @@  cxx_replaceable_global_alloc_fn (tree fndecl)
 static inline bool
 cxx_placement_new_fn (tree fndecl)
 {
-  if (cxx_dialect >= cxx20
-      && IDENTIFIER_NEW_OP_P (DECL_NAME (fndecl))
-      && CP_DECL_CONTEXT (fndecl) == global_namespace
-      && !DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fndecl)
-      && TREE_CODE (TREE_TYPE (fndecl)) == FUNCTION_TYPE)
-    {
-      tree first_arg = TREE_CHAIN (TYPE_ARG_TYPES (TREE_TYPE (fndecl)));
-      if (TREE_VALUE (first_arg) == ptr_type_node
-	  && TREE_CHAIN (first_arg) == void_list_node)
-	return true;
-    }
-  return false;
+  return (cxx_dialect >= cxx20 && std_placement_new_fn_p (fndecl));
 }
 
 /* Return true if FNDECL is std::construct_at.  */
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 92d1dba6a5c..fb04b6f1ae0 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7281,6 +7281,7 @@  extern tree build_offset_ref			(tree, tree, bool,
 extern tree throw_bad_array_new_length		(void);
 extern bool type_has_new_extended_alignment	(tree);
 extern unsigned malloc_alignment		(void);
+extern bool std_placement_new_fn_p		(tree);
 extern tree build_new_constexpr_heap_type	(tree, tree, tree);
 extern tree build_new				(location_t,
 						 vec<tree, va_gc> **, tree,
diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
index 62b3d6f6ce9..a11701002c8 100644
--- a/gcc/cp/init.cc
+++ b/gcc/cp/init.cc
@@ -2976,10 +2976,12 @@  malloc_alignment ()
 
 /* Determine whether an allocation function is a namespace-scope
    non-replaceable placement new function. See DR 1748.  */
-static bool
+bool
 std_placement_new_fn_p (tree alloc_fn)
 {
-  if (DECL_NAMESPACE_SCOPE_P (alloc_fn))
+  if (DECL_NAMESPACE_SCOPE_P (alloc_fn)
+      && IDENTIFIER_NEW_OP_P (DECL_NAME (alloc_fn))
+      && !DECL_IS_REPLACEABLE_OPERATOR_NEW_P (alloc_fn))
     {
       tree first_arg = TREE_CHAIN (TYPE_ARG_TYPES (TREE_TYPE (alloc_fn)));
       if (first_arg
diff --git a/gcc/testsuite/g++.dg/init/new54.C b/gcc/testsuite/g++.dg/init/new54.C
new file mode 100644
index 00000000000..fdff1b55f0d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/new54.C
@@ -0,0 +1,14 @@ 
+// PR c++/117463
+// { dg-do "compile" { target c++20 } }
+
+struct S {};
+void *operator new[] (unsigned long, // { dg-bogus "first parameter" "" { xfail *-*-* } }
+		      void void *volatile p); // { dg-error "two or more" }
+S *fun(void *p) {
+  return new(p) S[10];
+}
+
+void *operator new (decltype(sizeof(0)), // { dg-bogus "first parameter" "" { xfail *-*-* } }
+		    void void * p); // { dg-error "two or more" }
+void *p;
+auto t = new(p) int;