| Message ID | 20191123010321.GG2466@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | Fix attribute access issues | expand |
On November 23, 2019 2:03:21 AM GMT+01:00, Jakub Jelinek <jakub@redhat.com> 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? Ok. Richard. >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
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 >
--- 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;