This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch] fix warnings with -D_FORTIFY_SOURCE and -Wformat-security
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Matthias Klose <doko at ubuntu dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, fortran at gcc dot gnu dot org
- Date: Sun, 15 Mar 2009 23:33:40 +0000 (UTC)
- Subject: Re: [patch] fix warnings with -D_FORTIFY_SOURCE and -Wformat-security
- References: <49BD8445.6070705@ubuntu.com>
On Sun, 15 Mar 2009, Matthias Klose wrote:
> --- src/libcpp/macro.c~ 2008-09-20 19:50:30.000000000 +0200
> +++ src/libcpp/macro.c 2009-03-15 15:40:23.000000000 +0100
> @@ -1701,7 +1701,7 @@
> function-like macros, but not at the end. */
> if (following_paste_op)
> {
> - cpp_error (pfile, CPP_DL_ERROR, paste_op_error_msg);
> + cpp_error (pfile, CPP_DL_ERROR, "%s", paste_op_error_msg);
> return false;
> }
> break;
> @@ -1714,7 +1714,7 @@
> function-like macros, but not at the beginning. */
> if (macro->count == 1)
> {
> - cpp_error (pfile, CPP_DL_ERROR, paste_op_error_msg);
> + cpp_error (pfile, CPP_DL_ERROR, "%s", paste_op_error_msg);
> return false;
> }
>
These changes are wrong. cpp_error passes its third argument to be
translated (it's surrounded by N_ at the site where the string occurs),
but not subsequent arguments, so you would lose the translation of the
message and instead get a useless "%s" marked for translation.
> --- src/gcc/c-typeck.c~ 2009-02-21 08:44:48.000000000 +0100
> +++ src/gcc/c-typeck.c 2009-03-15 15:44:23.000000000 +0100
> @@ -2730,7 +2730,7 @@
> else if ((invalid_func_diag =
> targetm.calls.invalid_arg_for_unprototyped_fn (typelist, fundecl, val)))
> {
> - error (invalid_func_diag);
> + error ("%s", invalid_func_diag);
> return -1;
> }
> else
> @@ -2947,7 +2947,7 @@
> if ((invalid_op_diag
> = targetm.invalid_unary_op (code, TREE_TYPE (xarg))))
> {
> - error_at (location, invalid_op_diag);
> + error_at (location, "%s", invalid_op_diag);
> return error_mark_node;
> }
>
> @@ -8095,7 +8095,7 @@
> if ((invalid_op_diag
> = targetm.invalid_binary_op (code, type0, type1)))
> {
> - error_at (location, invalid_op_diag);
> + error_at (location, "%s", invalid_op_diag);
> return error_mark_node;
> }
>
Likewise. Furthermore, note that the diagnostics returned by these hooks
may use the %< and %> GCC-specific formats, which do not take arguments,
so they really do need to be passed as the correct argument to the
diagnostic function and interpreted as a format; moving the translation
earlier (calling error_at (location, "%s", _(invalid_op_diag))) won't
suffice.
I didn't check the rest of the patch, but in general such substitutions
for diagnostic functions are incorrect; the message needs to be passed
through translation exactly once (typically through being the msgid or
gmsgid argument to a diagnostic function) as well as being marked for
translation where the literal string appears (either with N_ etc. or
through being passed to a parameter called "msgid" or "gmsgid"). I expect
almost all the diagnostic changes in the patch are wrong.
--
Joseph S. Myers
joseph@codesourcery.com