diff mbox series

allow more strncat calls with -Wstringop-truncation (PR 85700)

Message ID a4a63845-f4d8-d132-6aea-a27645401749@gmail.com
State New
Headers show
Series allow more strncat calls with -Wstringop-truncation (PR 85700) | expand

Commit Message

Martin Sebor May 23, 2018, 1:40 a.m. UTC 
Here's another small refinement to -Wstringop-truncation to
avoid diagnosing more arguably "safe" cases of strncat() that
match the expected pattern of

   strncat (d, s, sizeof d - strlen (d) - 1);

such as

   extern char a[4];
   strncat (a, "12", sizeof d - strlen (a) - 1);

Since the bound is derived from the length of the destination
as GCC documents is the expected use, the call should probably
not be diagnosed even though truncation is possible.

The trouble with strncat is that it specifies a single count
that can be (and has been) used to specify either the remaining
space in the destination or the maximum number of characters
to append, but not both.  It's nearly impossible to tell for
certain which the author meant, and if it's safe, hence all
this fine-tuning.  I suspect this isn't the last tweak, either.

In any event, I'd like to commit the patch to both trunk and
gcc-8-branch.  The bug isn't marked regression but I suppose
it could (should) well be considered one.

Martin

Comments

Martin Sebor May 29, 2018, 4:19 p.m. UTC | #1 
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01189.html

On 05/22/2018 07:40 PM, Martin Sebor wrote:
> Here's another small refinement to -Wstringop-truncation to
> avoid diagnosing more arguably "safe" cases of strncat() that
> match the expected pattern of
>
>   strncat (d, s, sizeof d - strlen (d) - 1);
>
> such as
>
>   extern char a[4];
>   strncat (a, "12", sizeof d - strlen (a) - 1);
>
> Since the bound is derived from the length of the destination
> as GCC documents is the expected use, the call should probably
> not be diagnosed even though truncation is possible.
>
> The trouble with strncat is that it specifies a single count
> that can be (and has been) used to specify either the remaining
> space in the destination or the maximum number of characters
> to append, but not both.  It's nearly impossible to tell for
> certain which the author meant, and if it's safe, hence all
> this fine-tuning.  I suspect this isn't the last tweak, either.
>
> In any event, I'd like to commit the patch to both trunk and
> gcc-8-branch.  The bug isn't marked regression but I suppose
> it could (should) well be considered one.
>
> Martin
Martin Sebor June 4, 2018, 11:45 p.m. UTC | #2 
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01189.html

On 05/29/2018 10:19 AM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01189.html
>
> On 05/22/2018 07:40 PM, Martin Sebor wrote:
>> Here's another small refinement to -Wstringop-truncation to
>> avoid diagnosing more arguably "safe" cases of strncat() that
>> match the expected pattern of
>>
>>   strncat (d, s, sizeof d - strlen (d) - 1);
>>
>> such as
>>
>>   extern char a[4];
>>   strncat (a, "12", sizeof d - strlen (a) - 1);
>>
>> Since the bound is derived from the length of the destination
>> as GCC documents is the expected use, the call should probably
>> not be diagnosed even though truncation is possible.
>>
>> The trouble with strncat is that it specifies a single count
>> that can be (and has been) used to specify either the remaining
>> space in the destination or the maximum number of characters
>> to append, but not both.  It's nearly impossible to tell for
>> certain which the author meant, and if it's safe, hence all
>> this fine-tuning.  I suspect this isn't the last tweak, either.
>>
>> In any event, I'd like to commit the patch to both trunk and
>> gcc-8-branch.  The bug isn't marked regression but I suppose
>> it could (should) well be considered one.
>>
>> Martin
>
Martin Sebor June 11, 2018, 5:34 p.m. UTC | #3 
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01189.html

On 06/04/2018 05:45 PM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01189.html
>
> On 05/29/2018 10:19 AM, Martin Sebor wrote:
>> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01189.html
>>
>> On 05/22/2018 07:40 PM, Martin Sebor wrote:
>>> Here's another small refinement to -Wstringop-truncation to
>>> avoid diagnosing more arguably "safe" cases of strncat() that
>>> match the expected pattern of
>>>
>>>   strncat (d, s, sizeof d - strlen (d) - 1);
>>>
>>> such as
>>>
>>>   extern char a[4];
>>>   strncat (a, "12", sizeof d - strlen (a) - 1);
>>>
>>> Since the bound is derived from the length of the destination
>>> as GCC documents is the expected use, the call should probably
>>> not be diagnosed even though truncation is possible.
>>>
>>> The trouble with strncat is that it specifies a single count
>>> that can be (and has been) used to specify either the remaining
>>> space in the destination or the maximum number of characters
>>> to append, but not both.  It's nearly impossible to tell for
>>> certain which the author meant, and if it's safe, hence all
>>> this fine-tuning.  I suspect this isn't the last tweak, either.
>>>
>>> In any event, I'd like to commit the patch to both trunk and
>>> gcc-8-branch.  The bug isn't marked regression but I suppose
>>> it could (should) well be considered one.
>>>
>>> Martin
>>
>
Martin Sebor June 19, 2018, 2:49 a.m. UTC | #4 
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01189.html

On 06/11/2018 11:34 AM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01189.html
>
> On 06/04/2018 05:45 PM, Martin Sebor wrote:
>> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01189.html
>>
>> On 05/29/2018 10:19 AM, Martin Sebor wrote:
>>> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01189.html
>>>
>>> On 05/22/2018 07:40 PM, Martin Sebor wrote:
>>>> Here's another small refinement to -Wstringop-truncation to
>>>> avoid diagnosing more arguably "safe" cases of strncat() that
>>>> match the expected pattern of
>>>>
>>>>   strncat (d, s, sizeof d - strlen (d) - 1);
>>>>
>>>> such as
>>>>
>>>>   extern char a[4];
>>>>   strncat (a, "12", sizeof d - strlen (a) - 1);
>>>>
>>>> Since the bound is derived from the length of the destination
>>>> as GCC documents is the expected use, the call should probably
>>>> not be diagnosed even though truncation is possible.
>>>>
>>>> The trouble with strncat is that it specifies a single count
>>>> that can be (and has been) used to specify either the remaining
>>>> space in the destination or the maximum number of characters
>>>> to append, but not both.  It's nearly impossible to tell for
>>>> certain which the author meant, and if it's safe, hence all
>>>> this fine-tuning.  I suspect this isn't the last tweak, either.
>>>>
>>>> In any event, I'd like to commit the patch to both trunk and
>>>> gcc-8-branch.  The bug isn't marked regression but I suppose
>>>> it could (should) well be considered one.
>>>>
>>>> Martin
>>>
>>
>
Jeff Law June 23, 2018, 4:39 a.m. UTC | #5 
On 05/22/2018 07:40 PM, Martin Sebor wrote:
> Here's another small refinement to -Wstringop-truncation to
> avoid diagnosing more arguably "safe" cases of strncat() that
> match the expected pattern of
> 
>   strncat (d, s, sizeof d - strlen (d) - 1);
> 
> such as
> 
>   extern char a[4];
>   strncat (a, "12", sizeof d - strlen (a) - 1);
> 
> Since the bound is derived from the length of the destination
> as GCC documents is the expected use, the call should probably
> not be diagnosed even though truncation is possible.
> 
> The trouble with strncat is that it specifies a single count
> that can be (and has been) used to specify either the remaining
> space in the destination or the maximum number of characters
> to append, but not both.  It's nearly impossible to tell for
> certain which the author meant, and if it's safe, hence all
> this fine-tuning.  I suspect this isn't the last tweak, either.
> 
> In any event, I'd like to commit the patch to both trunk and
> gcc-8-branch.  The bug isn't marked regression but I suppose
> it could (should) well be considered one.
> 
> Martin
> 
> gcc-85700.diff
> 
> 
> PR tree-optimization/85700 - Spurious -Wstringop-truncation warning with strncat
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/85700
> 	* gimple-fold.c (gimple_fold_builtin_strncat): Adjust comment.
> 	* tree-ssa-strlen.c (is_strlen_related_p): Handle integer subtraction.
> 	(maybe_diag_stxncpy_trunc): Distinguish strncat from strncpy.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/85700
> 	* gcc.dg/Wstringop-truncation-3.c: New test.
OK for the trunk.  Sorry for the delay.

I don't see that strong of a case for backporting -- the distro rebuilds
with gcc-8 didn't trip over it (at least not in a -Werror build).  It
doesn't fix a codegen issue and we've only got the one report (unknown
what source the report originated with).  But you can certainly ask
Jakub or Richi.

jeff
diff mbox series

Patch

PR tree-optimization/85700 - Spurious -Wstringop-truncation warning with strncat

gcc/ChangeLog:

	PR tree-optimization/85700
	* gimple-fold.c (gimple_fold_builtin_strncat): Adjust comment.
	* tree-ssa-strlen.c (is_strlen_related_p): Handle integer subtraction.
	(maybe_diag_stxncpy_trunc): Distinguish strncat from strncpy.

gcc/testsuite/ChangeLog:

	PR tree-optimization/85700
	* gcc.dg/Wstringop-truncation-3.c: New test.

diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index b45798c..c37abe1 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -2062,10 +2062,12 @@  gimple_fold_builtin_strncat (gimple_stmt_iterator *gsi)
   if (!nowarn && cmpsrc == 0)
     {
       tree fndecl = gimple_call_fndecl (stmt);
-
-      /* To avoid certain truncation the specified bound should also
-	 not be equal to (or less than) the length of the source.  */
       location_t loc = gimple_location (stmt);
+
+      /* To avoid possible overflow the specified bound should also
+	 not be equal to the length of the source, even when the size
+	 of the destination is unknown (it's not an uncommon mistake
+	 to specify as the bound to strncpy the length of the source).  */
       if (warning_at (loc, OPT_Wstringop_overflow_,
 		      "%G%qD specified bound %E equals source length",
 		      stmt, fndecl, len))
diff --git a/gcc/testsuite/gcc.dg/Wstringop-truncation-3.c b/gcc/testsuite/gcc.dg/Wstringop-truncation-3.c
new file mode 100644
index 0000000..f394863
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-truncation-3.c
@@ -0,0 +1,63 @@ 
+/* PR tree-optimization/85700 - Spurious -Wstringop-truncation warning
+   with strncat
+   { dg-do compile }
+   { dg-options "-O2 -Wno-stringop-overflow -Wstringop-truncation -ftrack-macro-expansion=0" } */
+
+#define NOIPA __attribute__ ((noipa))
+#define strncat __builtin_strncat
+#define strlen __builtin_strlen
+
+extern char a4[4], b4[4], ax[];
+
+NOIPA void cat_a4_s1_1 (void)
+{
+  /* There is no truncation here but since the bound of 1 equals
+     the length of the source string it's likely a mistake that
+     could cause overflow so it's diagnosed by -Wstringop-overflow */
+  strncat (a4, "1", 1);
+}
+
+NOIPA void cat_a4_s1_2 (void)
+{
+  strncat (a4, "1", 2);
+}
+
+NOIPA void cat_a4_s1_3 (void)
+{
+  strncat (a4, "1", 3);
+}
+
+NOIPA void cat_a4_s1_4 (void)
+{
+  /* There is no truncation here but since the bound of 1 equals
+     the length of the source string it's likely a mistake that
+     could cause overflow so it's diagnosed by -Wstringop-overflow */
+  strncat (a4, "1", 4);
+}
+
+NOIPA void cat_a4_s1_5 (void)
+{
+  /* A bound in excess of the destination size is diagnosed by
+     -Wstringop-overflow.  */
+  strncat (a4, "1", 5);
+}
+
+NOIPA void cat_a4_s1_dlen (void)
+{
+  strncat (a4, "1", sizeof a4 - strlen (a4) - 1);
+}
+
+NOIPA void cat_a4_s2_dlen (void)
+{
+  strncat (a4, "12", sizeof a4 - strlen (a4) - 1);  /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+}
+
+NOIPA void cat_a4_b4_dlen (void)
+{
+  strncat (a4, b4, sizeof a4 - strlen (a4) - 1);  /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+}
+
+NOIPA void cat_ax_b4_dlen (void)
+{
+  strncat (ax, b4, 32 - strlen (ax) - 1);  /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+}
diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index 556c5bc..22a17d6 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
@@ -1778,6 +1778,15 @@  is_strlen_related_p (tree src, tree len)
       return is_strlen_related_p (src, rhs1);
     }
 
+  if (tree rhs2 = gimple_assign_rhs2 (def_stmt))
+    {
+      /* Integer subtraction is considered strlen-related when both
+	 arguments are integers and second one is strlen-related.  */
+      rhstype = TREE_TYPE (rhs2);
+      if (INTEGRAL_TYPE_P (rhstype) && code == MINUS_EXPR)
+	return is_strlen_related_p (src, rhs2);
+    }
+
   return false;
 }
 
@@ -1969,6 +1978,12 @@  maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi, tree src, tree cnt)
 
       gcall *call = as_a <gcall *> (stmt);
 
+      /* Set to true for strncat whose bound is derived from the length
+	 of the destination (the expected usage pattern).  */
+      bool cat_dstlen_bounded = false;
+      if (DECL_FUNCTION_CODE (func) == BUILT_IN_STRNCAT)
+	cat_dstlen_bounded = is_strlen_related_p (dst, cnt);
+
       if (lenrange[0] == cntrange[1] && cntrange[0] == cntrange[1])
 	return warning_n (callloc, OPT_Wstringop_truncation,
 			  cntrange[0].to_uhwi (),
@@ -1998,7 +2013,8 @@  maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi, tree src, tree cnt)
 			     call, func, cntrange[0].to_uhwi (),
 			     cntrange[1].to_uhwi (), lenrange[0].to_uhwi ());
 	}
-      else if (wi::geu_p (lenrange[1], cntrange[1]))
+      else if (!cat_dstlen_bounded
+	       && wi::geu_p (lenrange[1], cntrange[1]))
 	{
 	  /* The longest string is longer than the upper bound of
 	     the count so the truncation is possible.  */
@@ -2012,13 +2028,14 @@  maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi, tree src, tree cnt)
 			      call, func, cnt, lenrange[1].to_uhwi ());
 
 	  return warning_at (callloc, OPT_Wstringop_truncation,
-			     "%G%qD output may be truncated copying between %wu "
-			     "and %wu bytes from a string of length %wu",
+			     "%G%qD output may be truncated copying between "
+			     "%wu and %wu bytes from a string of length %wu",
 			     call, func, cntrange[0].to_uhwi (),
 			     cntrange[1].to_uhwi (), lenrange[1].to_uhwi ());
 	}
 
-      if (cntrange[0] != cntrange[1]
+      if (!cat_dstlen_bounded
+	  && cntrange[0] != cntrange[1]
 	  && wi::leu_p (cntrange[0], lenrange[0])
 	  && wi::leu_p (cntrange[1], lenrange[0] + 1))
 	{