diff mbox series

ubsan: Avoid narrowing of multiply for -fsanitize=signed-integer-overflow [PR108256]

Message ID Y7VCJRmHC/U+F53U@tucnak
State Unresolved
Headers
Series ubsan: Avoid narrowing of multiply for -fsanitize=signed-integer-overflow [PR108256] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek Jan. 4, 2023, 9:08 a.m. UTC 
  Hi!

We shouldn't narrow multiplications originally done in signed types,
because the original multiplication might overflow but the narrowed
one will be done in unsigned arithmetics and will never overflow.

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

2023-01-04  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/108256
	* convert.cc (do_narrow): Punt for MULT_EXPR if original
	type doesn't wrap around and -fsanitize=signed-integer-overflow
	is on.
	* fold-const.cc (fold_unary_loc) <CASE_CONVERT>: Likewise.

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


	Jakub

Comments

Richard Biener Jan. 4, 2023, 9:19 a.m. UTC | #1 
> Am 04.01.2023 um 10:09 schrieb Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org>:
> 
> Hi!
> 
> We shouldn't narrow multiplications originally done in signed types,
> because the original multiplication might overflow but the narrowed
> one will be done in unsigned arithmetics and will never overflow.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok. Richard 

> 2023-01-04  Jakub Jelinek  <jakub@redhat.com>
> 
>    PR sanitizer/108256
>    * convert.cc (do_narrow): Punt for MULT_EXPR if original
>    type doesn't wrap around and -fsanitize=signed-integer-overflow
>    is on.
>    * fold-const.cc (fold_unary_loc) <CASE_CONVERT>: Likewise.
> 
>    * c-c++-common/ubsan/pr108256.c: New test.
> 
> --- gcc/convert.cc.jj    2023-01-02 09:32:25.123245723 +0100
> +++ gcc/convert.cc    2023-01-03 10:02:36.309706050 +0100
> @@ -384,6 +384,14 @@ do_narrow (location_t loc,
>       && sanitize_flags_p (SANITIZE_SI_OVERFLOW))
>     return NULL_TREE;
> 
> +  /* Similarly for multiplication, but in that case it can be
> +     problematic even if typex is unsigned type - 0xffff * 0xffff
> +     overflows in int.  */
> +  if (ex_form == MULT_EXPR
> +      && !TYPE_OVERFLOW_WRAPS (TREE_TYPE (expr))
> +      && sanitize_flags_p (SANITIZE_SI_OVERFLOW))
> +    return NULL_TREE;
> +
>   /* But now perhaps TYPEX is as wide as INPREC.
>      In that case, do nothing special here.
>      (Otherwise would recurse infinitely in convert.  */
> --- gcc/fold-const.cc.jj    2023-01-02 09:32:32.756135438 +0100
> +++ gcc/fold-const.cc    2023-01-03 10:30:05.492239455 +0100
> @@ -9574,7 +9574,9 @@ fold_unary_loc (location_t loc, enum tre
>       if (INTEGRAL_TYPE_P (type)
>      && TREE_CODE (op0) == MULT_EXPR
>      && INTEGRAL_TYPE_P (TREE_TYPE (op0))
> -      && TYPE_PRECISION (type) < TYPE_PRECISION (TREE_TYPE (op0)))
> +      && TYPE_PRECISION (type) < TYPE_PRECISION (TREE_TYPE (op0))
> +      && (TYPE_OVERFLOW_WRAPS (TREE_TYPE (op0))
> +          || !sanitize_flags_p (SANITIZE_SI_OVERFLOW)))
>    {
>      /* Be careful not to introduce new overflows.  */
>      tree mult_type;
> --- gcc/testsuite/c-c++-common/ubsan/pr108256.c.jj    2023-01-03 10:14:49.064284638 +0100
> +++ gcc/testsuite/c-c++-common/ubsan/pr108256.c    2023-01-03 10:43:58.838326443 +0100
> @@ -0,0 +1,27 @@
> +/* PR sanitizer/108256 */
> +/* { dg-do run { target { lp64 || ilp32 } } } */
> +/* { dg-options "-fsanitize=signed-integer-overflow" } */
> +
> +unsigned short
> +foo (unsigned short x, unsigned short y)
> +{
> +  return x * y;
> +}
> +
> +unsigned short
> +bar (unsigned short x, unsigned short y)
> +{
> +  int r = x * y;
> +  return r;
> +}
> +
> +int
> +main ()
> +{
> +  volatile unsigned short a = foo (0xffff, 0xffff);
> +  volatile unsigned short b = bar (0xfffe, 0xfffe);
> +  return 0;
> +}
> +
> +/* { dg-output "signed integer overflow: 65535 \\\* 65535 cannot be represented in type 'int'\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "\[^\n\r]*signed integer overflow: 65534 \\\* 65534 cannot be represented in type 'int'" } */
> 
>    Jakub
>
diff mbox series

Patch

--- gcc/convert.cc.jj	2023-01-02 09:32:25.123245723 +0100
+++ gcc/convert.cc	2023-01-03 10:02:36.309706050 +0100
@@ -384,6 +384,14 @@  do_narrow (location_t loc,
       && sanitize_flags_p (SANITIZE_SI_OVERFLOW))
     return NULL_TREE;
 
+  /* Similarly for multiplication, but in that case it can be
+     problematic even if typex is unsigned type - 0xffff * 0xffff
+     overflows in int.  */
+  if (ex_form == MULT_EXPR
+      && !TYPE_OVERFLOW_WRAPS (TREE_TYPE (expr))
+      && sanitize_flags_p (SANITIZE_SI_OVERFLOW))
+    return NULL_TREE;
+
   /* But now perhaps TYPEX is as wide as INPREC.
      In that case, do nothing special here.
      (Otherwise would recurse infinitely in convert.  */
--- gcc/fold-const.cc.jj	2023-01-02 09:32:32.756135438 +0100
+++ gcc/fold-const.cc	2023-01-03 10:30:05.492239455 +0100
@@ -9574,7 +9574,9 @@  fold_unary_loc (location_t loc, enum tre
       if (INTEGRAL_TYPE_P (type)
 	  && TREE_CODE (op0) == MULT_EXPR
 	  && INTEGRAL_TYPE_P (TREE_TYPE (op0))
-	  && TYPE_PRECISION (type) < TYPE_PRECISION (TREE_TYPE (op0)))
+	  && TYPE_PRECISION (type) < TYPE_PRECISION (TREE_TYPE (op0))
+	  && (TYPE_OVERFLOW_WRAPS (TREE_TYPE (op0))
+	      || !sanitize_flags_p (SANITIZE_SI_OVERFLOW)))
 	{
 	  /* Be careful not to introduce new overflows.  */
 	  tree mult_type;
--- gcc/testsuite/c-c++-common/ubsan/pr108256.c.jj	2023-01-03 10:14:49.064284638 +0100
+++ gcc/testsuite/c-c++-common/ubsan/pr108256.c	2023-01-03 10:43:58.838326443 +0100
@@ -0,0 +1,27 @@ 
+/* PR sanitizer/108256 */
+/* { dg-do run { target { lp64 || ilp32 } } } */
+/* { dg-options "-fsanitize=signed-integer-overflow" } */
+
+unsigned short
+foo (unsigned short x, unsigned short y)
+{
+  return x * y;
+}
+
+unsigned short
+bar (unsigned short x, unsigned short y)
+{
+  int r = x * y;
+  return r;
+}
+
+int
+main ()
+{
+  volatile unsigned short a = foo (0xffff, 0xffff);
+  volatile unsigned short b = bar (0xfffe, 0xfffe);
+  return 0;
+}
+
+/* { dg-output "signed integer overflow: 65535 \\\* 65535 cannot be represented in type 'int'\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*signed integer overflow: 65534 \\\* 65534 cannot be represented in type 'int'" } */