[PATCH] add missing overflow check to stpcpy (PR 79222)

[PATCH] add missing overflow check to stpcpy (PR 79222)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 188 bytes --]

In implementing the -Wstringop-overflow warning I missed stpcpy.
The attached patch adds the required checking.  Given how simple
it is, does it qualify for GCC 7 despite stage 4?

Martin

[-- Attachment #2: gcc-79222.diff --]
[-- Type: text/x-patch, Size: 4183 bytes --]

PR middle-end/79222 - missing -Wstringop-overflow= on a stpcpy overflow

gcc/ChangeLog:

	PR middle-end/79222
	* builtins.c (expand_builtin_stpcpy): Check for buffer overflow.

gcc/testsuite/ChangeLog:

	PR middle-end/79222
	* gcc.dg/builtin-stringop-chk-4.c: Add test cases.
	* gcc.dg/pr79222.c: New test.

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 244844)
+++ gcc/builtins.c	(working copy)
@@ -3612,6 +3615,13 @@ expand_builtin_stpcpy (tree exp, rtx target, machi
   dst = CALL_EXPR_ARG (exp, 0);
   src = CALL_EXPR_ARG (exp, 1);
 
+  if (warn_stringop_overflow)
+    {
+      tree destsize = compute_dest_size (dst, warn_stringop_overflow - 1);
+      check_sizes (OPT_Wstringop_overflow_,
+		   exp, /*size=*/NULL_TREE, /*maxlen=*/NULL_TREE, src, destsize);
+    }
+
   /* If return value is ignored, transform stpcpy into strcpy.  */
   if (target == const0_rtx && builtin_decl_implicit (BUILT_IN_STRCPY))
     {
Index: gcc/testsuite/gcc.dg/builtin-stringop-chk-4.c
===================================================================
--- gcc/testsuite/gcc.dg/builtin-stringop-chk-4.c	(revision 244844)
+++ gcc/testsuite/gcc.dg/builtin-stringop-chk-4.c	(working copy)
@@ -41,6 +41,9 @@ extern char* (strcat)(char*, const char*);
 #define strncat(d, s, n) (strncat ((d), (s), (n)), sink ((d)))
 extern char* (strncat)(char*, const char*, size_t);
 
+#define stpcpy(d, s) (stpcpy ((d), (s)), sink ((d)))
+extern char* (stpcpy)(char*, const char*);
+
 #define strcpy(d, s) (strcpy ((d), (s)), sink ((d)))
 extern char* (strcpy)(char*, const char*);
 
@@ -349,6 +352,49 @@ void test_strcpy_range (void)
   strcpy (buf + 17, S (0));   /* { dg-warning "writing 1 byte into a region of size 0 " } */
 }
 
+/* Same test_strcpy but for stpcpy.  Verify that stpcpy with an unknown
+   source string doesn't cause warnings unless the destination has zero
+   size.  */
+
+void test_stpcpy (const char *src)
+{
+  struct A { char a[2]; char b[3]; } a;
+
+  stpcpy (a.a, src);
+  stpcpy (a.a + 1, src);
+
+  stpcpy (a.a + 2, src);    /* { dg-warning "writing at least 1 byte into a region of size 0 " "stpcpy into empty substring" { xfail *-*-* } } */
+
+  /* This does work.  */
+  stpcpy (a.a + 5, src);    /* { dg-warning "writing at least 1 byte into a region of size 0 " } */
+
+  /* As does this.  */
+  stpcpy (a.a + 17, src);    /* { dg-warning "writing at least 1 byte into a region of size 0 " } */
+}
+
+/* Same test_strcpy but for stpcpy.  Test stpcpy with a non-constant source
+   string of length in a known range.  */
+
+void test_stpcpy_range (void)
+{
+  char buf[5];
+
+  stpcpy (buf, S (0));
+  stpcpy (buf, S (1));
+  stpcpy (buf, S (2));
+  stpcpy (buf, S (4));
+  stpcpy (buf, S (5));   /* { dg-warning "writing 6 bytes into a region of size 5 " } */
+  stpcpy (buf, S (6));   /* { dg-warning "writing 7 bytes into a region of size 5 " } */
+  stpcpy (buf, S (7));   /* { dg-warning "writing 8 bytes into a region of size 5 " } */
+  stpcpy (buf, S (8));   /* { dg-warning "writing 9 bytes into a region of size 5 " } */
+  stpcpy (buf, S (9));   /* { dg-warning "writing 10 bytes into a region of size 5 " } */
+  stpcpy (buf, S (10));   /* { dg-warning "writing 11 bytes into a region of size 5 " } */
+
+  stpcpy (buf + 5, S (0));   /* { dg-warning "writing 1 byte into a region of size 0 " } */
+
+  stpcpy (buf + 17, S (0));   /* { dg-warning "writing 1 byte into a region of size 0 " } */
+}
+
 /* Test strncat with an argument referencing a non-constant string of
    lengths in a known range.  */
 
Index: gcc/testsuite/gcc.dg/pr79222.c
===================================================================
--- gcc/testsuite/gcc.dg/pr79222.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr79222.c	(working copy)
@@ -0,0 +1,11 @@
+/* PR middle-end/79222 - missing -Wstringop-overflow= on a stpcpy overflow
+   { dg-do compile }
+   { dg-options "-O2" } */
+
+char d[3];
+
+char* f (int i)
+{
+  const char *s = i < 0 ? "01234567" : "9876543210";
+  return __builtin_stpcpy (d, s);   /* { dg-warning ".__builtin_stpcpy. writing 9 bytes into a region of size 3 overflows the destination" } */
+}

Re: [PATCH] add missing overflow check to stpcpy (PR 79222)

[Jeff Law]

On 01/24/2017 08:36 PM, Martin Sebor wrote:
> In implementing the -Wstringop-overflow warning I missed stpcpy.
> The attached patch adds the required checking.  Given how simple
> it is, does it qualify for GCC 7 despite stage 4?
>
> Martin
>
> gcc-79222.diff
>
>
> PR middle-end/79222 - missing -Wstringop-overflow= on a stpcpy overflow
>
> gcc/ChangeLog:
>
> 	PR middle-end/79222
> 	* builtins.c (expand_builtin_stpcpy): Check for buffer overflow.
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/79222
> 	* gcc.dg/builtin-stringop-chk-4.c: Add test cases.
> 	* gcc.dg/pr79222.c: New test.
I think we should probably defer this as well.  We're really at a place 
where we'd really prefer to avoid making unnecessary changes.  My worry 
with this patch is we could start seeing code which used stpcpy start 
giving warnings.  It's unlikely (as I don't think stpcpy is used that 
much), but I'd rather wait.

And ISTM that we may be better keeping the tests separate (ie, a new 
test file rather than included in stringop-chk-4.c).  It makes merges 
easier if something does need to change in that file for gcc-7.

Our focus really needs to be on issues that affect our ability to make a 
release.  ie, regressions.

Jeff

Re: [PATCH] add missing overflow check to stpcpy (PR 79222)

[Jeff Law]

On 01/24/2017 08:36 PM, Martin Sebor wrote:
> In implementing the -Wstringop-overflow warning I missed stpcpy.
> The attached patch adds the required checking.  Given how simple
> it is, does it qualify for GCC 7 despite stage 4?
> 
> Martin
> 
> gcc-79222.diff
> 
> 
> PR middle-end/79222 - missing -Wstringop-overflow= on a stpcpy overflow
> 
> gcc/ChangeLog:
> 
> 	PR middle-end/79222
> 	* builtins.c (expand_builtin_stpcpy): Check for buffer overflow.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR middle-end/79222
> 	* gcc.dg/builtin-stringop-chk-4.c: Add test cases.
> 	* gcc.dg/pr79222.c: New test.
This is OK for the trunk.

jeff