Re: -Wformat-security warnings generated in gcc build

[Prathamesh Kulkarni <bilbotheelffriend at gmail dot com>]

On Thu, Jan 23, 2014 at 5:02 PM, Dodji Seketeli <dodji@redhat.com> wrote:
> "Joseph S. Myers" <joseph@codesourcery.com> a écrit:
>
>> On Wed, 22 Jan 2014, Prathamesh Kulkarni wrote:
>>
>>> Unfortunately, I am not clear on how to check for format specifiers in string.
>>> Should I do it manually by checking the format string for specifiers
>>> and call abort if found a no-argument specifier,
>>> or is there a better way to do it ?
>>
>> I'll leave it to Dodji as diagnostics maintainer to advise on the best way
>> to implement error_at_no_args.
>
> Thanks for the heads-up, Joseph.
>
> Assuming we want to do something more than just segfaulting if
> error_at_no_args is given a format specifier that needs an argument, I
> think the function pretty-print.c:pp_format is the place where the
> core of the change has to be done.  Basically, that function walks the
> format string and expands format specifiers.
>
> Thus, in that function, if a format specifier needs an argument (that
> is, each time we try to access text->args_ptr) but we are in a context
> where we want to accept only no-argument specifiers, we can call abort
> or internal_error with a meaningful error message.
>
> I guess we can assume that we know that we are in a
> oo-argument-specifier context if text->args_ptr is NULL in pp_format()
> That text->args_ptr is actually the va_ap that you (Prathamesh) set to
> NULL when you did:
>
>     void
>     error_at_no_args (location_t loc, const char *gmsgid)
>     {
>       diagnostic_info diagnostic;
>       diagnostic_set_info (&diagnostic, gmsgid, NULL, loc, DK_ERROR);
>                                                 ^^^^
>                                                  ^
>                                                  |
>                                          here: __|
>
>       report_diagnostic (&diagnostic);
>     }
>
>
> And then, just keep the error_at_no_args() implementation like what
> you did already.

Shall it be correct then to replace calls to error() and friends,
taking only format string with no-argument specifiers
to error_at_no_args() ? (similarly we shall need warning_at_no_args,
pedwarn_no_args, etc.)

>
> Also, you'd need to modify cp/error.c:cp_printer in a similar way, to
> issue an internal_error each time we try to access a null test->args_ptr.

Shall check for text->args_ptr be required in each case label of
argument specifier in pp_format()
and client-specific functions like cp_printer() ?

eg:
case 'c':
    gcc_assert (text->args_ptr);
    pp_character (pp, va_arg (*text->args_ptr, int));
    break;

>
> But then, I guess some people might argue that we could as well let
> the code as is and just segfault on accessing a NULL test->args_ptr.
> I would personally lean towards aborting with a meaningful error
> message, but I'd like to hear what others think about this.
>
> I hope this helps.
>
> --
>                 Dodji