diff mbox series

c++, v2: Fix get_member_function_from_ptrfunc with -fsanitize=bounds [PR116449]

Message ID ZtiVAaH17KaG7U9y@tucnak
State New
Headers
Series c++, v2: Fix get_member_function_from_ptrfunc with -fsanitize=bounds [PR116449] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed

Commit Message

Jakub Jelinek Sept. 4, 2024, 5:12 p.m. UTC 
  On Wed, Sep 04, 2024 at 12:34:04PM -0400, Jason Merrill wrote:
> > So, one possibility would be to call save_expr unconditionally in
> > get_member_function_from_ptrfunc as well.
> > 
> > Or build a TARGET_EXPR (force_target_expr or similar).
> 
> Yes.  I don't have a strong preference between the two.

Here is a patch that uses save_expr but uses it still conditionally,
doesn't make sense to use it for the common case of just decls, there is
nothing to unshare in that case.

Passed the test, ok if it passes full bootstrap/regtest?

2024-09-04  Jakub Jelinek  <jakub@redhat.com>

	PR c++/116449
	* typeck.cc (get_member_function_from_ptrfunc): Use save_expr
	on instance_ptr and function even if it doesn't have side-effects,
	as long as it isn't a decl.

	* g++.dg/ubsan/pr116449.C: New test.



	Jakub

Comments

Franz Sirl Sept. 4, 2024, 8:31 p.m. UTC | #1 
Am 2024-09-04 um 19:12 schrieb Jakub Jelinek:
> On Wed, Sep 04, 2024 at 12:34:04PM -0400, Jason Merrill wrote:
>>> So, one possibility would be to call save_expr unconditionally in
>>> get_member_function_from_ptrfunc as well.
>>>
>>> Or build a TARGET_EXPR (force_target_expr or similar).
>>
>> Yes.  I don't have a strong preference between the two.
> 
> Here is a patch that uses save_expr but uses it still conditionally,
> doesn't make sense to use it for the common case of just decls, there is
> nothing to unshare in that case.
> 
> Passed the test, ok if it passes full bootstrap/regtest?
> 
> 2024-09-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/116449
> 	* typeck.cc (get_member_function_from_ptrfunc): Use save_expr
> 	on instance_ptr and function even if it doesn't have side-effects,
> 	as long as it isn't a decl.
> 
> 	* g++.dg/ubsan/pr116449.C: New test.
> 
> --- gcc/cp/typeck.cc.jj	2024-09-02 17:07:30.115098114 +0200
> +++ gcc/cp/typeck.cc	2024-09-04 19:08:24.127490242 +0200
> @@ -4188,10 +4188,21 @@ get_member_function_from_ptrfunc (tree *
>         if (!nonvirtual && is_dummy_object (instance_ptr))
>   	nonvirtual = true;
>   
> -      if (TREE_SIDE_EFFECTS (instance_ptr))
> -	instance_ptr = instance_save_expr = save_expr (instance_ptr);
> +      /* Use save_expr even when instance_ptr doesn't have side-effects,
> +	 unless it is a simple decl (save_expr won't do anything on
> +	 constants), so that we don't ubsan instrument the expression
> +	 multiple times.  See PR116449.  */
> +      if (TREE_SIDE_EFFECTS (instance_ptr) || !DECL_P (instance_ptr))
> +	{
> +	  instance_save_expr = save_expr (instance_ptr);
> +	  if (instance_save_expr == instance_ptr)
> +	    instance_save_expr = NULL_TREE;
> +	  else
> +	    instance_ptr = instance_save_expr;
> +	}
>   
> -      if (TREE_SIDE_EFFECTS (function))
> +      /* See above comment.  */
> +      if (TREE_SIDE_EFFECTS (function) || !DECL_P (function))
>   	function = save_expr (function);

Hmm, it just occured to me, how about adding !NONVIRTUAL here? When 
NONVIRTUAL is true, there is no conditional stmt at all, or?

Franz
diff mbox series

Patch

--- gcc/cp/typeck.cc.jj	2024-09-02 17:07:30.115098114 +0200
+++ gcc/cp/typeck.cc	2024-09-04 19:08:24.127490242 +0200
@@ -4188,10 +4188,21 @@  get_member_function_from_ptrfunc (tree *
       if (!nonvirtual && is_dummy_object (instance_ptr))
 	nonvirtual = true;
 
-      if (TREE_SIDE_EFFECTS (instance_ptr))
-	instance_ptr = instance_save_expr = save_expr (instance_ptr);
+      /* Use save_expr even when instance_ptr doesn't have side-effects,
+	 unless it is a simple decl (save_expr won't do anything on
+	 constants), so that we don't ubsan instrument the expression
+	 multiple times.  See PR116449.  */
+      if (TREE_SIDE_EFFECTS (instance_ptr) || !DECL_P (instance_ptr))
+	{
+	  instance_save_expr = save_expr (instance_ptr);
+	  if (instance_save_expr == instance_ptr)
+	    instance_save_expr = NULL_TREE;
+	  else
+	    instance_ptr = instance_save_expr;
+	}
 
-      if (TREE_SIDE_EFFECTS (function))
+      /* See above comment.  */
+      if (TREE_SIDE_EFFECTS (function) || !DECL_P (function))
 	function = save_expr (function);
 
       /* Start by extracting all the information from the PMF itself.  */
--- gcc/testsuite/g++.dg/ubsan/pr116449.C.jj	2024-09-04 18:58:46.106764285 +0200
+++ gcc/testsuite/g++.dg/ubsan/pr116449.C	2024-09-04 18:58:46.106764285 +0200
@@ -0,0 +1,14 @@ 
+// PR c++/116449
+// { dg-do compile }
+// { dg-options "-O2 -Wall -fsanitize=undefined" }
+
+struct C { void foo (int); void bar (); int c[16]; };
+typedef void (C::*P) ();
+struct D { P d; };
+static D e[1] = { { &C::bar } };
+
+void
+C::foo (int x)
+{
+  (this->*e[c[x]].d) ();
+}