[PATCH] handle invalid calls to built-ins with no prototype (PR 83603)

[PATCH] handle invalid calls to built-ins with no prototype (PR 83603)

[Martin Sebor]

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

The -Wrestrict code assumes that built-ins are called with
the correct number of arguments.  When this isn't so it
crashes.  The attached patch avoids the ICE due to this
error.

There are outstanding assumptions that the type of actual
arguments to built-ins declared without a prototype matches
the expected type.  Those will be corrected in a subsequent
patch.

Martin

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

PR tree-optimization/83603 - ICE in builtin_memref at gcc/gimple-ssa-warn-restrict.c:238

gcc/ChangeLog:

	PR tree-optimization/83603
	* calls.c (maybe_warn_nonstring_arg): Avoid accessing function
	arguments past the endof the argument list in functions declared
	without a prototype.
	* gimple-ssa-warn-restrict.c (wrestrict_dom_walker::check_call):
	Avoid checking when arguments are null.

diff --git a/gcc/calls.c b/gcc/calls.c
index 9b7e118..0cfc20a 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -1606,6 +1606,8 @@ maybe_warn_nonstring_arg (tree fndecl, tree exp)
 
   bool with_bounds = CALL_WITH_BOUNDS_P (exp);
 
+  unsigned nargs = call_expr_nargs (exp);
+
   /* The bound argument to a bounded string function like strncpy.  */
   tree bound = NULL_TREE;
 
@@ -1620,12 +1622,20 @@ maybe_warn_nonstring_arg (tree fndecl, tree exp)
     case BUILT_IN_STRNCASECMP:
     case BUILT_IN_STRNCPY:
     case BUILT_IN_STRNCPY_CHK:
-      bound = CALL_EXPR_ARG (exp, with_bounds ? 4 : 2);
-      break;
+      {
+	unsigned argno = with_bounds ? 4 : 2;
+	if (argno < nargs)
+	  bound = CALL_EXPR_ARG (exp, argno);
+	break;
+      }
 
     case BUILT_IN_STRNDUP:
-      bound = CALL_EXPR_ARG (exp, with_bounds ? 2 : 1);
-      break;
+      {
+	unsigned argno = with_bounds ? 2 : 1;
+	if (argno < nargs)
+	  bound = CALL_EXPR_ARG (exp, argno);
+	break;
+      }
 
     default:
       break;
@@ -1645,6 +1655,11 @@ maybe_warn_nonstring_arg (tree fndecl, tree exp)
 
   for (unsigned argno = 0; ; ++argno, function_args_iter_next (&it))
     {
+      /* Avoid iterating past the declared argument in a call
+	 to function declared without a prototype.  */
+      if (argno >= nargs)
+	break;
+
       tree argtype = function_args_iter_cond (&it);
       if (!argtype)
 	break;
diff --git a/gcc/gimple-ssa-warn-restrict.c b/gcc/gimple-ssa-warn-restrict.c
index ebec381..a2a29cd 100644
--- a/gcc/gimple-ssa-warn-restrict.c
+++ b/gcc/gimple-ssa-warn-restrict.c
@@ -1710,7 +1710,9 @@ wrestrict_dom_walker::check_call (gcall *call)
   if (!dstwr && strfun)
     dstwr = size_one_node;
 
-  if (check_bounds_or_overlap (call, dst, src, dstwr, NULL_TREE))
+  /* DST and SRC can be null for a call with an insufficient number
+     of arguments to a built-in function declared without a protype.  */
+  if (!dst || !src || check_bounds_or_overlap (call, dst, src, dstwr, NULL_TREE))
     return;
 
   /* Avoid diagnosing the call again.  */
diff --git a/gcc/testsuite/gcc.dg/Wrestrict-4.c b/gcc/testsuite/gcc.dg/Wrestrict-4.c
new file mode 100644
index 0000000..f2398ef
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wrestrict-4.c
@@ -0,0 +1,110 @@
+/* PR tree-optimization/83603 - ICE in builtin_memref at
+   gcc/gimple-ssa-warn-restrict.c:238
+   Test to verify that invalid calls to built-in functions declared
+   without a prototype don't cause an ICE.
+   { dg-do compile }
+   { dg-options "-O2 -Warray-bounds -Wrestrict" } */
+
+void* memcpy ();
+void* memmove ();
+char* stpcpy ();
+char* strcat ();
+char* strcpy ();
+char* strncat ();
+char* strncpy ();
+
+void* test_memcpy_0 ()
+{
+  return memcpy ();
+}
+
+void* test_memcpy_1 (void *d)
+{
+  return memcpy (d);
+}
+
+void* test_memcpy_2 (void *d, const void *s)
+{
+  return memcpy (d, s);
+}
+
+
+void* test_memmove_0 ()
+{
+  return memmove ();
+}
+
+void* test_memmove_1 (void *d)
+{
+  return memmove (d);
+}
+
+void* test_memmove_2 (void *d, const void *s)
+{
+  return memmove (d, s);
+}
+
+
+void* test_stpcpy_0 ()
+{
+  return stpcpy ();
+}
+
+void* test_stpcpy_1 (char *d)
+{
+  return stpcpy (d);
+}
+
+
+char* test_strcat_0 ()
+{
+  return strcat ();
+}
+
+char* test_strcat_1 (char *d)
+{
+  return strcat (d);
+}
+
+
+void* test_strcpy_0 ()
+{
+  return strcpy ();
+}
+
+void* test_strcpy_1 (char *d)
+{
+  return strcpy (d);
+}
+
+
+char* test_strncat_0 ()
+{
+  return strncat ();
+}
+
+char* test_strncat_1 (char *d)
+{
+  return strncat (d);
+}
+
+char* test_strncat_2 (char *d, const char *s)
+{
+  return strncat (d, s);
+}
+
+
+void* test_strncpy_0 ()
+{
+  return strncpy ();
+}
+
+void* test_strncpy_1 (char *d)
+{
+  return strncpy (d);
+}
+
+void* test_strncpy_2 (char *d, const char *s)
+{
+  return strncpy (d, s);
+}

Re: [PATCH] handle invalid calls to built-ins with no prototype (PR 83603)

[Jeff Law]

On 01/01/2018 05:58 PM, Martin Sebor wrote:
> The -Wrestrict code assumes that built-ins are called with
> the correct number of arguments.  When this isn't so it
> crashes.  The attached patch avoids the ICE due to this
> error.
> 
> There are outstanding assumptions that the type of actual
> arguments to built-ins declared without a prototype matches
> the expected type.  Those will be corrected in a subsequent
> patch.
> 
> Martin
> 
> gcc-83603.diff
> 
> 
> PR tree-optimization/83603 - ICE in builtin_memref at gcc/gimple-ssa-warn-restrict.c:238
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/83603
> 	* calls.c (maybe_warn_nonstring_arg): Avoid accessing function
> 	arguments past the endof the argument list in functions declared
> 	without a prototype.
> 	* gimple-ssa-warn-restrict.c (wrestrict_dom_walker::check_call):
> 	Avoid checking when arguments are null.
OK.

Presumably we're not getting syntax errors precisely because we're
calling these functions without an in-scope prototype?

I wouldn't at all be surprised to find other passes mis-handling that
case.  The idioms you're using to get the arguments are similar to code
I've seen elsewhere -- it's probably just harder to trigger the failures
because analysis of the mem* or str* call is conditional on surrounding
context.

Jeff

Re: [PATCH] handle invalid calls to built-ins with no prototype (PR 83603)

[Martin Sebor]

On 01/02/2018 04:29 PM, Jeff Law wrote:
> On 01/01/2018 05:58 PM, Martin Sebor wrote:
>> The -Wrestrict code assumes that built-ins are called with
>> the correct number of arguments.  When this isn't so it
>> crashes.  The attached patch avoids the ICE due to this
>> error.
>>
>> There are outstanding assumptions that the type of actual
>> arguments to built-ins declared without a prototype matches
>> the expected type.  Those will be corrected in a subsequent
>> patch.
>>
>> Martin
>>
>> gcc-83603.diff
>>
>>
>> PR tree-optimization/83603 - ICE in builtin_memref at gcc/gimple-ssa-warn-restrict.c:238
>>
>> gcc/ChangeLog:
>>
>> 	PR tree-optimization/83603
>> 	* calls.c (maybe_warn_nonstring_arg): Avoid accessing function
>> 	arguments past the endof the argument list in functions declared
>> 	without a prototype.
>> 	* gimple-ssa-warn-restrict.c (wrestrict_dom_walker::check_call):
>> 	Avoid checking when arguments are null.
> OK.
>
> Presumably we're not getting syntax errors precisely because we're
> calling these functions without an in-scope prototype?

Right.

> I wouldn't at all be surprised to find other passes mis-handling that
> case.  The idioms you're using to get the arguments are similar to code
> I've seen elsewhere -- it's probably just harder to trigger the failures
> because analysis of the mem* or str* call is conditional on surrounding
> context.

Other passes may be relying on gimple_call_builtin_p to determine
whether a call is to a built-in.  The function returns false for
calls with incompatible arguments (not just when passing a pointer
where an integer is expected, for example, but also when passing
an int where a size_t is expected, such as in a call to memcpy),
and so it exempts many otherwise correct calls from checking (and
also from the benefits of optimization, which also implies less
thorough checking).

Martin