Re: [PATCH] Fix attribute access issues

[Martin Sebor <msebor@gmail.com>]

On 11/22/19 6:03 PM, Jakub Jelinek wrote:
> Hi!
> 
> On Thu, Nov 21, 2019 at 06:09:34PM -0700, Martin Sebor wrote:
>>>> 	PR middle-end/83859
>>>> 	* c-attribs.c (handle_access_attribute): New function.
>>>> 	(c_common_attribute_table): Add new attribute.
>>>> 	(get_argument_type): New function.
>>>> 	(append_access_attrs): New function.
> 
> I'm getting
> +FAIL: gcc.dg/Wstringop-overflow-24.c (internal compiler error)
> +FAIL: gcc.dg/Wstringop-overflow-24.c (test for excess errors)
> on i686-linux, while it succeeds on x86_64-linux.  On a closer look,
> there is a buffer overflow even on x86_64-linux as can be seen under
> valgrind, plus memory leak.
> 
> The buffer overflow is in append_access_attrs:
> ==9759== Command: ./cc1 -quiet -Wall Wstringop-overflow-24.c
> ==9759==
> ==9759== Invalid write of size 1
> ==9759==    at 0x483BD9F: strcpy (vg_replace_strmem.c:513)
> ==9759==    by 0xA11FF4: append_access_attrs(tree_node*, tree_node*, char const*, char, long*) (c-attribs.c:3934)
> ==9759==    by 0xA12AD3: handle_access_attribute(tree_node**, tree_node*, tree_node*, int, bool*) (c-attribs.c:4158)
> ==9759==    by 0x88E1BF: decl_attributes(tree_node**, tree_node*, int, tree_node*) (attribs.c:728)
> ==9759==    by 0x8A6A9B: c_decl_attributes(tree_node**, tree_node*, int) (c-decl.c:4944)
> ==9759==    by 0x8A6FE2: start_decl(c_declarator*, c_declspecs*, bool, tree_node*) (c-decl.c:5083)
> ==9759==    by 0x91CB15: c_parser_declaration_or_fndef(c_parser*, bool, bool, bool, bool, bool, tree_node**, vec<c_token, va_heap, vl_ptr>, bool, tree_node*, oacc_routine_data*, bool*) (c-parser.c:2216)
> ==9759==    by 0x91B742: c_parser_external_declaration(c_parser*) (c-parser.c:1690)
> ==9759==    by 0x91B25E: c_parser_translation_unit(c_parser*) (c-parser.c:1563)
> ==9759==    by 0x9590A4: c_parse_file() (c-parser.c:21524)
> ==9759==    by 0x9E308E: c_common_parse_file() (c-opts.c:1185)
> ==9759==    by 0x1211AEE: compile_file() (toplev.c:458)
> ==9759==  Address 0x5113f68 is 0 bytes after a block of size 8 alloc'd
> ==9759==    at 0x483880B: malloc (vg_replace_malloc.c:309)
> ==9759==    by 0x229BF17: xmalloc (xmalloc.c:147)
> ==9759==    by 0xA11FC0: append_access_attrs(tree_node*, tree_node*, char const*, char, long*) (c-attribs.c:3932)
> ==9759==    by 0xA12AD3: handle_access_attribute(tree_node**, tree_node*, tree_node*, int, bool*) (c-attribs.c:4158)
> ==9759==    by 0x88E1BF: decl_attributes(tree_node**, tree_node*, int, tree_node*) (attribs.c:728)
> ==9759==    by 0x8A6A9B: c_decl_attributes(tree_node**, tree_node*, int) (c-decl.c:4944)
> ==9759==    by 0x8A6FE2: start_decl(c_declarator*, c_declspecs*, bool, tree_node*) (c-decl.c:5083)
> ==9759==    by 0x91CB15: c_parser_declaration_or_fndef(c_parser*, bool, bool, bool, bool, bool, tree_node**, vec<c_token, va_heap, vl_ptr>, bool, tree_node*, oacc_routine_data*, bool*) (c-parser.c:2216)
> ==9759==    by 0x91B742: c_parser_external_declaration(c_parser*) (c-parser.c:1690)
> ==9759==    by 0x91B25E: c_parser_translation_unit(c_parser*) (c-parser.c:1563)
> ==9759==    by 0x9590A4: c_parse_file() (c-parser.c:21524)
> ==9759==    by 0x9E308E: c_common_parse_file() (c-opts.c:1185)
> If n2 != 0, newlen is computed as n1 + n2, but that doesn't take into
> account for the , that is added in between the two.
> 
> The following patch ought to fix both the buffer overflow (by adding 1 if n2
> is non-zero), memory leak (freeing newspec buffer after creating the string;
> I've considered using XALLOCAVEC instead, but I believe the string can be
> arbitrarily long on functions with thousands of arguments), using XNEWVEC
> instead of (type *) xmalloc, using auto_diagnostic_group to bind warning +
> inform together and fixes a typo in the documentation.
> 
> Ok for trunk if it passes bootstrap/regtest on x86_64-linux and i686-linux?

Thanks for the fix.

The buffer overflow enhancement I posted a couple of weeks ago has
logic to detect very simple forms of this bug:
https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00812.html
It knows how to figure out that the strcpy call below overflows:

   void* f1 (const char *s1)
   {
     __SIZE_TYPE__ n1 = __builtin_strlen (s1);
     char *s2 = __builtin_malloc (n1);
     __builtin_strcpy (s2, s1);
     return s2;
   }

   warning: ‘__builtin_strcpy’ writing one too many bytes into a region 
of a size that depends on ‘strlen’ [-Wstringop-overflow=]
       5 |   __builtin_strcpy (s2, s1);
         |   ^~~~~~~~~~~~~~~~~~~~~~~~~

Unfortunately, it doesn't yet know how to see the similar problem
in more complicated code such as this:

   void* f2 (const char *s1, const char *s2)
   {
     __SIZE_TYPE__ n1 = __builtin_strlen (s1);
     __SIZE_TYPE__ n2 = __builtin_strlen (s2);
     char *s3 = __builtin_malloc (n1 + n2);
     __builtin_strcpy (s3, s1);
     __builtin_strcat (s3, s2);
     return s3;
   }

Detecting that will take quite a bit more work.  Some sort of
a symbolic constraint evaluation engine.

Martin

> 
> 2019-11-23  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/83859
> 	* doc/extend.texi (attribute access): Fix a typo.
> 
> 	* c-attribs.c (append_access_attrs): Avoid buffer overflow.  Avoid
> 	memory leak.  Use XNEWVEC macro.  Use auto_diagnostic_group to
> 	group warning with inform together.
> 	(handle_access_attribute): Formatting fix.
> 
> --- gcc/doc/extend.texi.jj	2019-11-22 19:11:53.634970558 +0100
> +++ gcc/doc/extend.texi	2019-11-23 01:34:33.344849287 +0100
> @@ -2490,7 +2490,7 @@ The following attributes are supported o
>   
>   The @code{access} attribute enables the detection of invalid or unsafe
>   accesses by functions to which they apply to or their callers, as well
> -as wite-only accesses to objects that are never read from.  Such accesses
> +as write-only accesses to objects that are never read from.  Such accesses
>   may be diagnosed by warnings such as @option{-Wstringop-overflow},
>   @option{-Wunnitialized}, @option{-Wunused}, and others.
>   
> --- gcc/c-family/c-attribs.c.jj	2019-11-22 19:11:54.000000000 +0100
> +++ gcc/c-family/c-attribs.c	2019-11-23 01:44:50.306617000 +0100
> @@ -3840,7 +3840,7 @@ append_access_attrs (tree t, tree attrs,
>     if (idxs[1])
>       n2 = sprintf (attrspec + n1 + 1, "%u", (unsigned) idxs[1] - 1);
>   
> -  size_t newlen = n1 + n2;
> +  size_t newlen = n1 + n2 + !!n2;
>     char *newspec = attrspec;
>   
>     if (tree acs = lookup_attribute ("access", attrs))
> @@ -3869,6 +3869,7 @@ append_access_attrs (tree t, tree attrs,
>   	  if (*attrspec != pos[-1])
>   	    {
>   	      /* Mismatch in access mode.  */
> +	      auto_diagnostic_group d;
>   	      if (warning (OPT_Wattributes,
>   			   "attribute %qs mismatch with mode %qs",
>   			   attrstr,
> @@ -3884,6 +3885,7 @@ append_access_attrs (tree t, tree attrs,
>   	  if ((n2 && pos[n1 - 1] != ','))
>   	    {
>   	      /* Mismatch in the presence of the size argument.  */
> +	      auto_diagnostic_group d;
>   	      if (warning (OPT_Wattributes,
>   			   "attribute %qs positional argument 2 conflicts "
>   			   "with previous designation",
> @@ -3897,6 +3899,7 @@ append_access_attrs (tree t, tree attrs,
>   	  if (!n2 && pos[n1 - 1] == ',')
>   	    {
>   	      /* Mismatch in the presence of the size argument.  */
> +	      auto_diagnostic_group d;
>   	      if (warning (OPT_Wattributes,
>   			   "attribute %qs missing positional argument 2 "
>   			   "provided in previous designation",
> @@ -3910,6 +3913,7 @@ append_access_attrs (tree t, tree attrs,
>   	  if (n2 && strncmp (attrstr + n1 + 1, pos + n1, n2))
>   	    {
>   	      /* Mismatch in the value of the size argument.  */
> +	      auto_diagnostic_group d;
>   	      if (warning (OPT_Wattributes,
>   			   "attribute %qs mismatch positional argument "
>   			   "values %i and %i",
> @@ -3929,7 +3933,7 @@ append_access_attrs (tree t, tree attrs,
>   	attrspec[n1] = ',';
>   
>         size_t len = strlen (str);
> -      newspec = (char *) xmalloc (newlen + len + 1);
> +      newspec = XNEWVEC (char, newlen + len + 1);
>         strcpy (newspec, str);
>         strcpy (newspec + len, attrspec);
>         newlen += len;
> @@ -3938,7 +3942,10 @@ append_access_attrs (tree t, tree attrs,
>       /* Connect the two substrings formatted above into a single one.  */
>       attrspec[n1] = ',';
>   
> -  return build_string (newlen + 1, newspec);
> +  tree ret = build_string (newlen + 1, newspec);
> +  if (newspec != attrspec)
> +    XDELETEVEC (newspec);
> +  return ret;
>   }
>   
>   /* Handle the access attribute (read_only, write_only, and read_write).  */
> @@ -4168,7 +4175,8 @@ handle_access_attribute (tree *node, tre
>       {
>         /* Repeat for the previously declared type.  */
>         attrs = TYPE_ATTRIBUTES (TREE_TYPE (node[1]));
> -      tree new_attrs = append_access_attrs (node[1], attrs, attrstr, code, idxs);
> +      tree new_attrs
> +	= append_access_attrs (node[1], attrs, attrstr, code, idxs);
>         if (!new_attrs)
>   	return NULL_TREE;
>   
> 
> 
> 	Jakub
>