gcc-patches@gcc.gnu.org
[Top] [All Lists]

Re: [patch] fix warnings with -D_FORTIFY_SOURCE and -Wformat-security

Subject: Re: [patch] fix warnings with -D_FORTIFY_SOURCE and -Wformat-security
From: "Joseph S. Myers"
Date: Sun, 15 Mar 2009 23:33:40 +0000 UTC
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@xxxxxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>