[PATCH] c++: Unshare folded SAVE_EXPR arguments during cp_fold [PR112727]

[PATCH] c++: Unshare folded SAVE_EXPR arguments during cp_fold [PR112727]

[Jakub Jelinek]

Hi!

The following testcase is miscompiled because two ubsan instrumentations
run into each other.
The first one is the shift instrumentation.  Before the C++ FE calls
it, it wraps the 2 shift arguments with cp_save_expr, so that side-effects
in them aren't evaluated multiple times.  And, ubsan_instrument_shift
itself uses unshare_expr on any uses of the operands to make sure further
modifications in them don't affect other copies of them (the only not
unshared ones are the one the caller then uses for the actual operation
after the instrumentation, which means there is no tree sharing).

Now, if there are side-effects in the first operand like say function
call, cp_save_expr wraps it into a SAVE_EXPR, and ubsan_instrument_shift
in this mode emits something like
if (..., SAVE_EXPR <foo ()>, SAVE_EXPR <op1> > const)
  __ubsan_handle_shift_out_of_bounds (..., SAVE_EXPR <foo ()>, ...);
and caller adds
SAVE_EXPR <foo ()> << SAVE_EXPR <op1>
after it in a COMPOUND_EXPR.  So far so good.

If there are no side-effects and cp_save_expr doesn't create SAVE_EXPR,
everything is ok as well because of the unshare_expr.
We have
if (..., SAVE_EXPR <op1> > const)
  __ubsan_handle_shift_out_of_bounds (..., ptr->something[i], ...);
and
ptr->something[i] << SAVE_EXPR <op1>
where ptr->something[i] is unshared.

In the testcase below, the !x->s[j] ? 1 : 0 expression is wrapped initially
into a SAVE_EXPR though, and unshare_expr doesn't unshare SAVE_EXPRs nor
anything used in them for obvious reasons, so we end up with:
if (..., SAVE_EXPR <!(bool) VIEW_CONVERT_EXPR<const struct S *>(x)->s[j] ? 1 : 0>, SAVE_EXPR <op1> > const)
  __ubsan_handle_shift_out_of_bounds (..., SAVE_EXPR <!(bool) VIEW_CONVERT_EXPR<const struct S *>(x)->s[j] ? 1 : 0>, ...);
and
SAVE_EXPR <!(bool) VIEW_CONVERT_EXPR<const struct S *>(x)->s[j] ? 1 : 0> << SAVE_EXPR <op1>
So far good as well.  But later during cp_fold of the SAVE_EXPR we find
out that VIEW_CONVERT_EXPR<const struct S *>(x)->s[j] ? 0 : 1 is actually
invariant (has TREE_READONLY set) and so cp_fold simplifies the above to
if (..., SAVE_EXPR <op1> > const)
  __ubsan_handle_shift_out_of_bounds (..., (bool) VIEW_CONVERT_EXPR<const struct S *>(x)->s[j] ? 0 : 1, ...);
and
((bool) VIEW_CONVERT_EXPR<const struct S *>(x)->s[j] ? 0 : 1) << SAVE_EXPR <op1>
with the s[j] ARRAY_REFs and other expressions shared in between the two
uses (and obviously the expression optimized away from the COMPOUND_EXPR in
the if condition.

Then comes another ubsan instrumentation at genericization time,
this time to instrument the ARRAY_REFs with strict bounds checking,
and replaces the s[j] in there with s[.UBSAN_BOUNDS (0B, SAVE_EXPR<j>, 8), SAVE_EXPR<j>]
As the trees are shared, it does that just once though.
And as the if body is gimplified first, the SAVE_EXPR<j> is evaluated inside
of the if body and when it is used again after the if, it uses a potentially
uninitialized value of j.1 (always uninitialized if the shift count isn't
out of bounds).

The following patch fixes that by unshare_expr unsharing the folded argument
of a SAVE_EXPR if we've folded the SAVE_EXPR into an invariant and it is
used more than once.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2023-12-07  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/112727
	* cp-gimplify.cc (cp_fold): If SAVE_EXPR has been previously
	folded, unshare_expr what is returned.

	* c-c++-common/ubsan/pr112727.c: New test.

--- gcc/cp/cp-gimplify.cc.jj	2023-12-05 09:06:06.112878408 +0100
+++ gcc/cp/cp-gimplify.cc	2023-12-06 22:32:46.379370223 +0100
@@ -2906,7 +2906,14 @@ cp_fold (tree x, fold_flags_t flags)
     fold_cache = hash_map<tree, tree>::create_ggc (101);
 
   if (tree *cached = fold_cache->get (x))
-    return *cached;
+    {
+      /* unshare_expr doesn't recurse into SAVE_EXPRs.  If SAVE_EXPR's
+	 argument has been folded into a tree invariant, make sure it is
+	 unshared.  See PR112727.  */
+      if (TREE_CODE (x) == SAVE_EXPR && *cached != x)
+	return unshare_expr (*cached);
+      return *cached;
+    }
 
   uid_sensitive_constexpr_evaluation_checker c;
 
--- gcc/testsuite/c-c++-common/ubsan/pr112727.c.jj	2023-12-06 22:35:46.012819991 +0100
+++ gcc/testsuite/c-c++-common/ubsan/pr112727.c	2023-12-06 22:35:16.708236026 +0100
@@ -0,0 +1,17 @@
+/* PR sanitizer/112727 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fsanitize=shift-exponent,bounds-strict -Wuninitialized" } */
+
+#ifndef __cplusplus
+#define bool _Bool
+#endif
+
+struct S { bool s[8]; };
+
+void
+foo (const struct S *x)
+{
+  unsigned n = 0;
+  for (unsigned j = 0; j < 8; j++)
+    n |= ((!x->s[j]) ? 1 : 0) << (16 + j);
+}

	Jakub

Re: [PATCH] c++: Unshare folded SAVE_EXPR arguments during cp_fold [PR112727]

[Jason Merrill]

On 12/7/23 04:28, Jakub Jelinek wrote:
> Hi!
> 
> The following testcase is miscompiled because two ubsan instrumentations
> run into each other.
> The first one is the shift instrumentation.  Before the C++ FE calls
> it, it wraps the 2 shift arguments with cp_save_expr, so that side-effects
> in them aren't evaluated multiple times.  And, ubsan_instrument_shift
> itself uses unshare_expr on any uses of the operands to make sure further
> modifications in them don't affect other copies of them (the only not
> unshared ones are the one the caller then uses for the actual operation
> after the instrumentation, which means there is no tree sharing).
> 
> Now, if there are side-effects in the first operand like say function
> call, cp_save_expr wraps it into a SAVE_EXPR, and ubsan_instrument_shift
> in this mode emits something like
> if (..., SAVE_EXPR <foo ()>, SAVE_EXPR <op1> > const)
>    __ubsan_handle_shift_out_of_bounds (..., SAVE_EXPR <foo ()>, ...);
> and caller adds
> SAVE_EXPR <foo ()> << SAVE_EXPR <op1>
> after it in a COMPOUND_EXPR.  So far so good.
> 
> If there are no side-effects and cp_save_expr doesn't create SAVE_EXPR,
> everything is ok as well because of the unshare_expr.
> We have
> if (..., SAVE_EXPR <op1> > const)
>    __ubsan_handle_shift_out_of_bounds (..., ptr->something[i], ...);
> and
> ptr->something[i] << SAVE_EXPR <op1>
> where ptr->something[i] is unshared.
> 
> In the testcase below, the !x->s[j] ? 1 : 0 expression is wrapped initially
> into a SAVE_EXPR though, and unshare_expr doesn't unshare SAVE_EXPRs nor
> anything used in them for obvious reasons, so we end up with:
> if (..., SAVE_EXPR <!(bool) VIEW_CONVERT_EXPR<const struct S *>(x)->s[j] ? 1 : 0>, SAVE_EXPR <op1> > const)
>    __ubsan_handle_shift_out_of_bounds (..., SAVE_EXPR <!(bool) VIEW_CONVERT_EXPR<const struct S *>(x)->s[j] ? 1 : 0>, ...);
> and
> SAVE_EXPR <!(bool) VIEW_CONVERT_EXPR<const struct S *>(x)->s[j] ? 1 : 0> << SAVE_EXPR <op1>
> So far good as well.  But later during cp_fold of the SAVE_EXPR we find
> out that VIEW_CONVERT_EXPR<const struct S *>(x)->s[j] ? 0 : 1 is actually
> invariant (has TREE_READONLY set) and so cp_fold simplifies the above to
> if (..., SAVE_EXPR <op1> > const)
>    __ubsan_handle_shift_out_of_bounds (..., (bool) VIEW_CONVERT_EXPR<const struct S *>(x)->s[j] ? 0 : 1, ...);
> and
> ((bool) VIEW_CONVERT_EXPR<const struct S *>(x)->s[j] ? 0 : 1) << SAVE_EXPR <op1>
> with the s[j] ARRAY_REFs and other expressions shared in between the two
> uses (and obviously the expression optimized away from the COMPOUND_EXPR in
> the if condition.
> 
> Then comes another ubsan instrumentation at genericization time,
> this time to instrument the ARRAY_REFs with strict bounds checking,
> and replaces the s[j] in there with s[.UBSAN_BOUNDS (0B, SAVE_EXPR<j>, 8), SAVE_EXPR<j>]
> As the trees are shared, it does that just once though.
> And as the if body is gimplified first, the SAVE_EXPR<j> is evaluated inside
> of the if body and when it is used again after the if, it uses a potentially
> uninitialized value of j.1 (always uninitialized if the shift count isn't
> out of bounds).
> 
> The following patch fixes that by unshare_expr unsharing the folded argument
> of a SAVE_EXPR if we've folded the SAVE_EXPR into an invariant and it is
> used more than once.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Do we want to do the same for TARGET_EXPR, since those are handled like 
SAVE_EXPR in mostly_copy_tree_r?

> 2023-12-07  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR sanitizer/112727
> 	* cp-gimplify.cc (cp_fold): If SAVE_EXPR has been previously
> 	folded, unshare_expr what is returned.
> 
> 	* c-c++-common/ubsan/pr112727.c: New test.
> 
> --- gcc/cp/cp-gimplify.cc.jj	2023-12-05 09:06:06.112878408 +0100
> +++ gcc/cp/cp-gimplify.cc	2023-12-06 22:32:46.379370223 +0100
> @@ -2906,7 +2906,14 @@ cp_fold (tree x, fold_flags_t flags)
>       fold_cache = hash_map<tree, tree>::create_ggc (101);
>   
>     if (tree *cached = fold_cache->get (x))
> -    return *cached;
> +    {
> +      /* unshare_expr doesn't recurse into SAVE_EXPRs.  If SAVE_EXPR's
> +	 argument has been folded into a tree invariant, make sure it is
> +	 unshared.  See PR112727.  */
> +      if (TREE_CODE (x) == SAVE_EXPR && *cached != x)
> +	return unshare_expr (*cached);
> +      return *cached;
> +    }
>   
>     uid_sensitive_constexpr_evaluation_checker c;
>   
> --- gcc/testsuite/c-c++-common/ubsan/pr112727.c.jj	2023-12-06 22:35:46.012819991 +0100
> +++ gcc/testsuite/c-c++-common/ubsan/pr112727.c	2023-12-06 22:35:16.708236026 +0100
> @@ -0,0 +1,17 @@
> +/* PR sanitizer/112727 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fsanitize=shift-exponent,bounds-strict -Wuninitialized" } */
> +
> +#ifndef __cplusplus
> +#define bool _Bool
> +#endif
> +
> +struct S { bool s[8]; };
> +
> +void
> +foo (const struct S *x)
> +{
> +  unsigned n = 0;
> +  for (unsigned j = 0; j < 8; j++)
> +    n |= ((!x->s[j]) ? 1 : 0) << (16 + j);
> +}
> 
> 	Jakub
> 

Re: [PATCH] c++: Unshare folded SAVE_EXPR arguments during cp_fold [PR112727]

[Jakub Jelinek]

On Fri, Dec 08, 2023 at 11:51:19AM -0500, Jason Merrill wrote:
> Do we want to do the same for TARGET_EXPR, since those are handled like
> SAVE_EXPR in mostly_copy_tree_r?

In mostly_copy_tree_r yes, but I don't see cp_fold doing anything for
TARGET_EXPRs (like it does for SAVE_EXPRs), so I think TARGET_EXPRs stay
around until gimplification.

	Jakub

Re: [PATCH] c++: Unshare folded SAVE_EXPR arguments during cp_fold [PR112727]

[Jason Merrill]

On 12/8/23 12:35, Jakub Jelinek wrote:
> On Fri, Dec 08, 2023 at 11:51:19AM -0500, Jason Merrill wrote:
>> Do we want to do the same for TARGET_EXPR, since those are handled like
>> SAVE_EXPR in mostly_copy_tree_r?
> 
> In mostly_copy_tree_r yes, but I don't see cp_fold doing anything for
> TARGET_EXPRs (like it does for SAVE_EXPRs), so I think TARGET_EXPRs stay
> around until gimplification.

Makes sense, the patch is OK