On November 23, 2019 2:03:21 AM GMT+01:00, Jakub Jelinek <ja...@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 <ja...@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
