Fix gimple-ssa-sprintf.c caret related ICE (PR c/83448)

Message ID 20171221202917.GD2353@tucnak
State New
Headers show
Series
  • Fix gimple-ssa-sprintf.c caret related ICE (PR c/83448)
Related show

Commit Message

Jakub Jelinek Dec. 21, 2017, 8:29 p.m.
Hi!

If copying a substring without %s from the format string into the
destination does or might overflow, we try to point the caret at the
character in the format string that will cause the overflow.
In the first spot in maybe_warn this is only done if avail_range.min ==
avail_range.max and thus if we emit the warning later, the caret will
point into that substring of the format string (I've changed this hunk
anyway, so that we don't call set_caret_index with invalid offset in case we
won't do any diagnostics), but in the latter case we do it even if
avail_range.min != avail_range.max and want to put the caret on the maximum
because that will surely overflow (while the characters before might or
might not).  But especially in that case, we can set the caret even to
completely bogus spots and even overflow, set_caret_index expects an int
while navail, upper bound of a range, is UHWI and could be extremely large
in some cases.  Fixed by making sure to set the caret only if it falls
within the substring of the format string (not even pointing it to following
%whatever if any etc.).

The added testcase tests adds several caret position checks.

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

2017-12-21  Jakub Jelinek  <jakub@redhat.com>

	PR c/83448
	* gimple-ssa-sprintf.c (maybe_warn): Don't call set_caret_index
	if navail is >= dir.len.

	* gcc.c-torture/compile/pr83448.c: New test.
	* gcc.dg/tree-ssa/builtin-snprintf-warn-4.c: New test.


	Jakub

Comments

Martin Sebor Dec. 21, 2017, 10:24 p.m. | #1
On 12/21/2017 01:29 PM, Jakub Jelinek wrote:
> Hi!

>

> If copying a substring without %s from the format string into the

> destination does or might overflow, we try to point the caret at the

> character in the format string that will cause the overflow.

> In the first spot in maybe_warn this is only done if avail_range.min ==

> avail_range.max and thus if we emit the warning later, the caret will

> point into that substring of the format string (I've changed this hunk

> anyway, so that we don't call set_caret_index with invalid offset in case we

> won't do any diagnostics), but in the latter case we do it even if

> avail_range.min != avail_range.max and want to put the caret on the maximum

> because that will surely overflow (while the characters before might or

> might not).  But especially in that case, we can set the caret even to

> completely bogus spots and even overflow, set_caret_index expects an int

> while navail, upper bound of a range, is UHWI and could be extremely large

> in some cases.  Fixed by making sure to set the caret only if it falls

> within the substring of the format string (not even pointing it to following

> %whatever if any etc.).

>

> The added testcase tests adds several caret position checks.

>

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


Thanks for taking this on.  I looked at it briefly the other
day but didn't spend enough time on it to come up with a proper
fix.  Keeping the caret pointing at the overflowing character
as it should is definitely better than getting rid of it.

Martin

>

> 2017-12-21  Jakub Jelinek  <jakub@redhat.com>

>

> 	PR c/83448

> 	* gimple-ssa-sprintf.c (maybe_warn): Don't call set_caret_index

> 	if navail is >= dir.len.

>

> 	* gcc.c-torture/compile/pr83448.c: New test.

> 	* gcc.dg/tree-ssa/builtin-snprintf-warn-4.c: New test.

>

> --- gcc/gimple-ssa-sprintf.c.jj	2017-12-19 22:05:44.000000000 +0100

> +++ gcc/gimple-ssa-sprintf.c	2017-12-21 18:19:58.014529256 +0100

> @@ -2466,7 +2466,8 @@ maybe_warn (substring_loc &dirloc, locat

>  	  /* For plain character directives (i.e., the format string itself)

>  	     but not others, point the caret at the first character that's

>  	     past the end of the destination.  */

> -	  dirloc.set_caret_index (dirloc.get_caret_idx () + navail);

> +	  if (navail < dir.len)

> +	    dirloc.set_caret_index (dirloc.get_caret_idx () + navail);

>  	}

>

>        if (*dir.beg == '\0')

> @@ -2594,7 +2595,8 @@ maybe_warn (substring_loc &dirloc, locat

>        /* For plain character directives (i.e., the format string itself)

>  	 but not others, point the caret at the first character that's

>  	 past the end of the destination.  */

> -      dirloc.set_caret_index (dirloc.get_caret_idx () + navail);

> +      if (navail < dir.len)

> +	dirloc.set_caret_index (dirloc.get_caret_idx () + navail);

>      }

>

>    if (*dir.beg == '\0')

> --- gcc/testsuite/gcc.c-torture/compile/pr83448.c.jj	2017-12-21 18:37:39.759173019 +0100

> +++ gcc/testsuite/gcc.c-torture/compile/pr83448.c	2017-12-21 18:37:20.000000000 +0100

> @@ -0,0 +1,15 @@

> +/* PR c/83448 */

> +

> +char *a;

> +int b;

> +

> +void

> +foo (void)

> +{

> +  for (;;)

> +    {

> +      if (b < 0)

> +	foo ();

> +      __builtin_snprintf (a, b, "%*s", b, "");

> +    }

> +}

> --- gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-4.c.jj	2017-12-21 18:22:31.737596914 +0100

> +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-4.c	2017-12-21 18:34:33.195527986 +0100

> @@ -0,0 +1,46 @@

> +/* PR c/83448 */

> +/* { dg-do compile } */

> +/* { dg-options "-O2 -Wformat-truncation -fdiagnostics-show-caret" } */

> +

> +extern int snprintf (char *, __SIZE_TYPE__, const char *, ...);

> +

> +void

> +foo (char *a, char *b, char *c, int d, int e)

> +{

> +  snprintf (a, 7, "abc\\\123 efg");

> +  /* { dg-warning "directive output truncated writing 9 bytes into a region of size 7" "" { target *-*-* } .-1 }

> +     { dg-message ".snprintf. output 10 bytes into a destination of size 7" "note" { target *-*-* } .-2 }

> +     { dg-begin-multiline-output "" }

> +   snprintf (a, 7, "abc\\\123 efg");

> +                    ~~~~~~~~~~~^~

> +     { dg-end-multiline-output "" }

> +     { dg-begin-multiline-output "note" }

> +   snprintf (a, 7, "abc\\\123 efg");

> +   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> +     { dg-end-multiline-output "" } */

> +  d &= 63;

> +  d += 10;

> +  snprintf (b, 7, "a%dbcdefg", d);

> +  /* { dg-warning "'bcdefg' directive output truncated writing 6 bytes into a region of size 4" "" { target *-*-* } .-1 }

> +     { dg-message ".snprintf. output 10 bytes into a destination of size 7" "note" { target *-*-* } .-2 }

> +     { dg-begin-multiline-output "" }

> +   snprintf (b, 7, "a%dbcdefg", d);

> +                       ~~~~^~

> +     { dg-end-multiline-output "" }

> +     { dg-begin-multiline-output "note" }

> +   snprintf (b, 7, "a%dbcdefg", d);

> +   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> +     { dg-end-multiline-output "" } */

> +  e &= 127;

> +  snprintf (c, 7, "a%dbcdefgh", e);

> +  /* { dg-warning "'bcdefgh' directive output truncated writing 7 bytes into a region of size between 3 and 5" "" { target *-*-* } .-1 }

> +     { dg-message ".snprintf. output between 10 and 12 bytes into a destination of size 7" "note" { target *-*-* } .-2 }

> +     { dg-begin-multiline-output "" }

> +   snprintf (c, 7, "a%dbcdefgh", e);

> +                       ~~~~~^~

> +     { dg-end-multiline-output "" }

> +     { dg-begin-multiline-output "note" }

> +   snprintf (c, 7, "a%dbcdefgh", e);

> +   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> +     { dg-end-multiline-output "" } */

> +}

>

> 	Jakub

>
Jeff Law Dec. 21, 2017, 11:05 p.m. | #2
On 12/21/2017 01:29 PM, Jakub Jelinek wrote:
> Hi!

> 

> If copying a substring without %s from the format string into the

> destination does or might overflow, we try to point the caret at the

> character in the format string that will cause the overflow.

> In the first spot in maybe_warn this is only done if avail_range.min ==

> avail_range.max and thus if we emit the warning later, the caret will

> point into that substring of the format string (I've changed this hunk

> anyway, so that we don't call set_caret_index with invalid offset in case we

> won't do any diagnostics), but in the latter case we do it even if

> avail_range.min != avail_range.max and want to put the caret on the maximum

> because that will surely overflow (while the characters before might or

> might not).  But especially in that case, we can set the caret even to

> completely bogus spots and even overflow, set_caret_index expects an int

> while navail, upper bound of a range, is UHWI and could be extremely large

> in some cases.  Fixed by making sure to set the caret only if it falls

> within the substring of the format string (not even pointing it to following

> %whatever if any etc.).

> 

> The added testcase tests adds several caret position checks.

> 

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

> 

> 2017-12-21  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR c/83448

> 	* gimple-ssa-sprintf.c (maybe_warn): Don't call set_caret_index

> 	if navail is >= dir.len.

> 

> 	* gcc.c-torture/compile/pr83448.c: New test.

> 	* gcc.dg/tree-ssa/builtin-snprintf-warn-4.c: New test.

OK.
jeff

Patch

--- gcc/gimple-ssa-sprintf.c.jj	2017-12-19 22:05:44.000000000 +0100
+++ gcc/gimple-ssa-sprintf.c	2017-12-21 18:19:58.014529256 +0100
@@ -2466,7 +2466,8 @@  maybe_warn (substring_loc &dirloc, locat
 	  /* For plain character directives (i.e., the format string itself)
 	     but not others, point the caret at the first character that's
 	     past the end of the destination.  */
-	  dirloc.set_caret_index (dirloc.get_caret_idx () + navail);
+	  if (navail < dir.len)
+	    dirloc.set_caret_index (dirloc.get_caret_idx () + navail);
 	}
 
       if (*dir.beg == '\0')
@@ -2594,7 +2595,8 @@  maybe_warn (substring_loc &dirloc, locat
       /* For plain character directives (i.e., the format string itself)
 	 but not others, point the caret at the first character that's
 	 past the end of the destination.  */
-      dirloc.set_caret_index (dirloc.get_caret_idx () + navail);
+      if (navail < dir.len)
+	dirloc.set_caret_index (dirloc.get_caret_idx () + navail);
     }
 
   if (*dir.beg == '\0')
--- gcc/testsuite/gcc.c-torture/compile/pr83448.c.jj	2017-12-21 18:37:39.759173019 +0100
+++ gcc/testsuite/gcc.c-torture/compile/pr83448.c	2017-12-21 18:37:20.000000000 +0100
@@ -0,0 +1,15 @@ 
+/* PR c/83448 */
+
+char *a;
+int b;
+
+void
+foo (void)
+{
+  for (;;)
+    {
+      if (b < 0)
+	foo ();
+      __builtin_snprintf (a, b, "%*s", b, "");
+    }
+}
--- gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-4.c.jj	2017-12-21 18:22:31.737596914 +0100
+++ gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-4.c	2017-12-21 18:34:33.195527986 +0100
@@ -0,0 +1,46 @@ 
+/* PR c/83448 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wformat-truncation -fdiagnostics-show-caret" } */
+
+extern int snprintf (char *, __SIZE_TYPE__, const char *, ...);
+
+void
+foo (char *a, char *b, char *c, int d, int e)
+{
+  snprintf (a, 7, "abc\\\123 efg");
+  /* { dg-warning "directive output truncated writing 9 bytes into a region of size 7" "" { target *-*-* } .-1 }
+     { dg-message ".snprintf. output 10 bytes into a destination of size 7" "note" { target *-*-* } .-2 }
+     { dg-begin-multiline-output "" }
+   snprintf (a, 7, "abc\\\123 efg");
+                    ~~~~~~~~~~~^~
+     { dg-end-multiline-output "" }
+     { dg-begin-multiline-output "note" }
+   snprintf (a, 7, "abc\\\123 efg");
+   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+     { dg-end-multiline-output "" } */
+  d &= 63;
+  d += 10;
+  snprintf (b, 7, "a%dbcdefg", d);
+  /* { dg-warning "'bcdefg' directive output truncated writing 6 bytes into a region of size 4" "" { target *-*-* } .-1 }
+     { dg-message ".snprintf. output 10 bytes into a destination of size 7" "note" { target *-*-* } .-2 }
+     { dg-begin-multiline-output "" }
+   snprintf (b, 7, "a%dbcdefg", d);
+                       ~~~~^~
+     { dg-end-multiline-output "" }
+     { dg-begin-multiline-output "note" }
+   snprintf (b, 7, "a%dbcdefg", d);
+   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+     { dg-end-multiline-output "" } */
+  e &= 127;
+  snprintf (c, 7, "a%dbcdefgh", e);
+  /* { dg-warning "'bcdefgh' directive output truncated writing 7 bytes into a region of size between 3 and 5" "" { target *-*-* } .-1 }
+     { dg-message ".snprintf. output between 10 and 12 bytes into a destination of size 7" "note" { target *-*-* } .-2 }
+     { dg-begin-multiline-output "" }
+   snprintf (c, 7, "a%dbcdefgh", e);
+                       ~~~~~^~
+     { dg-end-multiline-output "" }
+     { dg-begin-multiline-output "note" }
+   snprintf (c, 7, "a%dbcdefgh", e);
+   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+     { dg-end-multiline-output "" } */
+}