This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Qing Zhao <qing dot zhao at oracle dot com>, "fortran at gcc dot gnu dot org" <fortran at gcc dot gnu dot org>
- Cc: gcc-patches <gcc-patches at gcc dot gnu dot org>, jeff Law <law at redhat dot com>, Richard Biener <rguenther at suse dot de>, Thomas Koenig <tkoenig at netcologne dot de>
- Date: Tue, 12 Dec 2017 09:20:18 +0100
- Subject: Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments
- Authentication-results: sourceware.org; auth=none
- References: <A7C37470-B6C8-4731-B719-0839AA8D9279@oracle.com>
On Mon, Dec 4, 2017 at 4:34 PM, Qing Zhao <qing.zhao@oracle.com> wrote:
> Hi,
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79538 <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79538>
> missing -Wformat-overflow with %s and non-member array arguments
>
> -Wformat-overflow uses the routine "get_range_strlen" to decide the
> maximum string length, however, currently "get_range_strlen" misses
> the handling of non-member arrays.
>
> Adding the handling of non-member array resolves the issue.
> Adding test case pr79538.c into gcc.dg.
>
> During gcc bootstrap, 2 source files (c-family/c-cppbuiltin.c,
> fortran/class.c) were detected new warnings by -Wformat-overflow
> due to the new handling of non-member array in "get_range_strlen".
> in order to avoid these new warnings and continue with bootstrap,
> updating these 2 files to avoid the warnings.
>
> in c-family/c-cppbuiltin.c, the warning is following:
>
> ../../latest_gcc_2/gcc/c-family/c-cppbuiltin.c:1627:15: note:
> ‘sprintf’ output 2 or more bytes (assuming 257) into a destination
> of size 256
> sprintf (buf1, "%s=%s", macro, buf2);
> ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> in the above, buf1 and buf2 are declared as:
> char buf1[256], buf2[256];
>
> i.e, buf1 and buf2 have same size. adjusting the size of buf1 and
> buf2 resolves the warning.
>
> fortran/class.c has the similar issue as above. Instead of adjusting
> size of the buffers, replacing sprintf with xasprintf.
>
> bootstraped and tested on both X86 and aarch64. no regression.
>
> Okay for trunk?
>
> thanks.
>
> Qing
>
> ===========================================================
>
> gcc/ChangeLog
>
> 2017-11-30 Qing Zhao <qing.zhao@oracle.com <mailto:qing.zhao@oracle.com>>
>
> PR middle_end/79538
> * gimple-fold.c (get_range_strlen): Add the handling of non-member array.
>
> gcc/fortran/ChangeLog
>
> 2017-11-30 Qing Zhao <qing.zhao@oracle.com <mailto:qing.zhao@oracle.com>>
>
> PR middle_end/79538
> * class.c (gfc_build_class_symbol): Replace call to
> sprintf with xasprintf to avoid format-overflow warning.
> (generate_finalization_wrapper): Likewise.
> (gfc_find_derived_vtab): Likewise.
> (find_intrinsic_vtab): Likewise.
>
>
> gcc/c-family/ChangeLog
>
> 2017-11-30 Qing Zhao <qing.zhao@oracle.com <mailto:qing.zhao@oracle.com>>
>
> PR middle_end/79538
> * c-cppbuiltin.c (builtin_define_with_hex_fp_value):
> Adjust the size of buf1 and buf2, add a new buf to avoid
> format-overflow warning.
>
> gcc/testsuite/ChangeLog
>
> 2017-11-30 Qing Zhao <qing.zhao@oracle.com <mailto:qing.zhao@oracle.com>>
>
> PR middle_end/79538
> * gcc.dg/pr79538.c: New test.
>
> ---
> gcc/c-family/c-cppbuiltin.c | 10 ++++-----
> gcc/fortran/class.c | 49 ++++++++++++++++++++++++------------------
> gcc/gimple-fold.c | 13 +++++++++++
> gcc/testsuite/gcc.dg/pr79538.c | 23 ++++++++++++++++++++
> 4 files changed, 69 insertions(+), 26 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/pr79538.c
>
> diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c
> index 2ac9616..9e33aed 100644
> --- a/gcc/c-family/c-cppbuiltin.c
> +++ b/gcc/c-family/c-cppbuiltin.c
> @@ -1613,7 +1613,7 @@ builtin_define_with_hex_fp_value (const char *macro,
> const char *fp_cast)
> {
> REAL_VALUE_TYPE real;
> - char dec_str[64], buf1[256], buf2[256];
> + char dec_str[64], buf[256], buf1[128], buf2[64];
>
> /* This is very expensive, so if possible expand them lazily. */
> if (lazy_hex_fp_value_count < 12
> @@ -1656,11 +1656,11 @@ builtin_define_with_hex_fp_value (const char *macro,
>
> /* Assemble the macro in the following fashion
> macro = fp_cast [dec_str fp_suffix] */
> - sprintf (buf1, "%s%s", dec_str, fp_suffix);
> - sprintf (buf2, fp_cast, buf1);
> - sprintf (buf1, "%s=%s", macro, buf2);
> + sprintf (buf2, "%s%s", dec_str, fp_suffix);
> + sprintf (buf1, fp_cast, buf2);
> + sprintf (buf, "%s=%s", macro, buf1);
>
> - cpp_define (parse_in, buf1);
> + cpp_define (parse_in, buf);
> }
This change looks ok.
> /* Return a string constant for the suffix for a value of type TYPE
> diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c
> index ebbd41b..a08fb8d 100644
> --- a/gcc/fortran/class.c
> +++ b/gcc/fortran/class.c
> @@ -602,7 +602,8 @@ bool
> gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
> gfc_array_spec **as)
> {
> - char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1];
> + char tname[GFC_MAX_SYMBOL_LEN+1];
> + char *name;
> gfc_symbol *fclass;
> gfc_symbol *vtab;
> gfc_component *c;
> @@ -633,17 +634,17 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
> rank = !(*as) || (*as)->rank == -1 ? GFC_MAX_DIMENSIONS : (*as)->rank;
> get_unique_hashed_string (tname, ts->u.derived);
> if ((*as) && attr->allocatable)
> - sprintf (name, "__class_%s_%d_%da", tname, rank, (*as)->corank);
> + name = xasprintf ("__class_%s_%d_%da", tname, rank, (*as)->corank);
> else if ((*as) && attr->pointer)
> - sprintf (name, "__class_%s_%d_%dp", tname, rank, (*as)->corank);
> + name = xasprintf ("__class_%s_%d_%dp", tname, rank, (*as)->corank);
> else if ((*as))
> - sprintf (name, "__class_%s_%d_%dt", tname, rank, (*as)->corank);
> + name = xasprintf ("__class_%s_%d_%dt", tname, rank, (*as)->corank);
> else if (attr->pointer)
> - sprintf (name, "__class_%s_p", tname);
> + name = xasprintf ("__class_%s_p", tname);
> else if (attr->allocatable)
> - sprintf (name, "__class_%s_a", tname);
> + name = xasprintf ("__class_%s_a", tname);
> else
> - sprintf (name, "__class_%s_t", tname);
> + name = xasprintf ("__class_%s_t", tname);
>
> if (ts->u.derived->attr.unlimited_polymorphic)
> {
> @@ -738,6 +739,7 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
> ts->u.derived = fclass;
> attr->allocatable = attr->pointer = attr->dimension = attr->codimension = 0;
> (*as) = NULL;
> + free (name);
> return true;
> }
>
> @@ -1527,7 +1529,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns,
> gfc_component *comp;
> gfc_namespace *sub_ns;
> gfc_code *last_code, *block;
> - char name[GFC_MAX_SYMBOL_LEN+1];
> + char *name;
> bool finalizable_comp = false;
> bool expr_null_wrapper = false;
> gfc_expr *ancestor_wrapper = NULL, *rank;
> @@ -1606,7 +1608,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns,
> sub_ns->resolved = 1;
>
> /* Set up the procedure symbol. */
> - sprintf (name, "__final_%s", tname);
> + name = xasprintf ("__final_%s", tname);
> gfc_get_symbol (name, sub_ns, &final);
> sub_ns->proc_name = final;
> final->attr.flavor = FL_PROCEDURE;
> @@ -2172,6 +2174,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns,
> gfc_free_expr (rank);
> vtab_final->initializer = gfc_lval_expr_from_sym (final);
> vtab_final->ts.interface = final;
> + free (name);
> }
>
>
> @@ -2239,10 +2242,11 @@ gfc_find_derived_vtab (gfc_symbol *derived)
>
> if (ns)
> {
> - char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1];
> + char tname[GFC_MAX_SYMBOL_LEN+1];
> + char *name;
>
> get_unique_hashed_string (tname, derived);
> - sprintf (name, "__vtab_%s", tname);
> + name = xasprintf ("__vtab_%s", tname);
>
> /* Look for the vtab symbol in various namespaces. */
> if (gsym && gsym->ns)
> @@ -2270,7 +2274,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
> vtab->attr.vtab = 1;
> vtab->attr.access = ACCESS_PUBLIC;
> gfc_set_sym_referenced (vtab);
> - sprintf (name, "__vtype_%s", tname);
> + name = xasprintf ("__vtype_%s", tname);
>
> gfc_find_symbol (name, ns, 0, &vtype);
> if (vtype == NULL)
> @@ -2373,7 +2377,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
> else
> {
> /* Construct default initialization variable. */
> - sprintf (name, "__def_init_%s", tname);
> + name = xasprintf ("__def_init_%s", tname);
> gfc_get_symbol (name, ns, &def_init);
> def_init->attr.target = 1;
> def_init->attr.artificial = 1;
> @@ -2406,7 +2410,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
> ns->contained = sub_ns;
> sub_ns->resolved = 1;
> /* Set up procedure symbol. */
> - sprintf (name, "__copy_%s", tname);
> + name = xasprintf ("__copy_%s", tname);
> gfc_get_symbol (name, sub_ns, ©);
> sub_ns->proc_name = copy;
> copy->attr.flavor = FL_PROCEDURE;
> @@ -2483,7 +2487,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
> ns->contained = sub_ns;
> sub_ns->resolved = 1;
> /* Set up procedure symbol. */
> - sprintf (name, "__deallocate_%s", tname);
> + name = xasprintf ("__deallocate_%s", tname);
> gfc_get_symbol (name, sub_ns, &dealloc);
> sub_ns->proc_name = dealloc;
> dealloc->attr.flavor = FL_PROCEDURE;
> @@ -2532,6 +2536,7 @@ have_vtype:
> vtab->ts.u.derived = vtype;
> vtab->value = gfc_default_initializer (&vtab->ts);
> }
> + free (name);
> }
>
> found_sym = vtab;
> @@ -2623,13 +2628,14 @@ find_intrinsic_vtab (gfc_typespec *ts)
>
> if (ns)
> {
> - char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1];
> -
> + char tname[GFC_MAX_SYMBOL_LEN+1];
> + char *name;
> +
> /* Encode all types as TYPENAME_KIND_ including especially character
> arrays, whose length is now consistently stored in the _len component
> of the class-variable. */
> sprintf (tname, "%s_%d_", gfc_basic_typename (ts->type), ts->kind);
> - sprintf (name, "__vtab_%s", tname);
> + name = xasprintf ("__vtab_%s", tname);
>
> /* Look for the vtab symbol in the top-level namespace only. */
> gfc_find_symbol (name, ns, 0, &vtab);
> @@ -2646,7 +2652,7 @@ find_intrinsic_vtab (gfc_typespec *ts)
> vtab->attr.vtab = 1;
> vtab->attr.access = ACCESS_PUBLIC;
> gfc_set_sym_referenced (vtab);
> - sprintf (name, "__vtype_%s", tname);
> + name = xasprintf ("__vtype_%s", tname);
>
> gfc_find_symbol (name, ns, 0, &vtype);
> if (vtype == NULL)
> @@ -2722,12 +2728,12 @@ find_intrinsic_vtab (gfc_typespec *ts)
> c->tb->ppc = 1;
>
> if (ts->type != BT_CHARACTER)
> - sprintf (name, "__copy_%s", tname);
> + name = xasprintf ("__copy_%s", tname);
> else
> {
> /* __copy is always the same for characters.
> Check to see if copy function already exists. */
> - sprintf (name, "__copy_character_%d", ts->kind);
> + name = xasprintf ("__copy_character_%d", ts->kind);
> contained = ns->contained;
> for (; contained; contained = contained->sibling)
> if (contained->proc_name
> @@ -2796,6 +2802,7 @@ find_intrinsic_vtab (gfc_typespec *ts)
> vtab->ts.u.derived = vtype;
> vtab->value = gfc_default_initializer (&vtab->ts);
> }
> + free (name);
It looks like this might be in a performance critical lookup path
where we'd really
like to avoid allocating/freeing memory. Why's GFC_MAX_SYMBOL_LEN+1 not
enough? Leaving for fortran folks to comment - note you should CC
fortran@gcc.gnu.org
for fortran patches (done).
> }
>
> found_sym = vtab;
> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> index 353a46e..fef1969 100644
> --- a/gcc/gimple-fold.c
> +++ b/gcc/gimple-fold.c
> @@ -1323,6 +1323,19 @@ get_range_strlen (tree arg, tree length[2], bitmap *visited, int type,
> the array could have zero length. */
> *minlen = ssize_int (0);
> }
> +
> + if (VAR_P (arg)
> + && TREE_CODE (TREE_TYPE (arg)) == ARRAY_TYPE)
> + {
> + val = TYPE_SIZE_UNIT (TREE_TYPE (arg));
> + if (!val || integer_zerop (val))
val might be non-constant so you also need to check TREE_CODE (val) !=
INTEGER_CST here.
> + return false;
> + val = fold_build2 (MINUS_EXPR, TREE_TYPE (val), val,
> + integer_one_node);
val = wide_int_to_tree (TREE_TYPE (val), wi::minus (wi::to_wide (val), 1));
you pass a possibly bogus type of 1 to fold_build2 above so the wide-int variant
is prefered.
The gimple-fold.c changes are ok with that change.
Richard.
> + /* Set the minimum size to zero since the string in
> + the array could have zero length. */
> + *minlen = ssize_int (0);
> + }
> }
>
> if (!val)
> diff --git a/gcc/testsuite/gcc.dg/pr79538.c b/gcc/testsuite/gcc.dg/pr79538.c
> new file mode 100644
> index 0000000..c5a631e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr79538.c
> @@ -0,0 +1,23 @@
> +/* PR middle-end/79538 - missing -Wformat-overflow with %s and non-member array arguments
> + { dg-do compile }
> + { dg-options "-O2 -Wformat-overflow" } */
> +
> +char a3[3];
> +char a4[4];
> +char d[3];
> +
> +void g (int i)
> +{
> + const char *s = i < 0 ? a3 : a4;
> + __builtin_sprintf (d, "%s", s); /* { dg-warning ".__builtin_sprintf. may write a terminating nul past the end of the destination" } */
> + return;
> +}
> +
> +void f ()
> +{
> + char des[3];
> + char src[] = "abcd";
> + __builtin_sprintf (des, "%s", src); /* { dg-warning "directive writing up to 4 bytes into a region of size 3" } */
> + return;
> +}
> +
> --
> 1.9.1