Re: "format not a string literal" warnings

Greg Stein wrote:
> On Mon, Nov 10, 2008 at 5:36 AM, Hyrum K. Wright
> <hyrum_wright_at_mail.utexas.edu> wrote:
>> Hi all.
>>
>> Since getting rid of the deprecated warnings, I've noticed a new set of warnings
>> showing themselves to me. This is with gcc 4.3.2 on Ubuntu Intrepid. For example:
>>
>> subversion/svnlook/main.c: In function 'print_diff_tree':
>> subversion/svnlook/main.c:967: warning: format not a string literal and no
>> format arguments
>> subversion/svnlook/main.c:986: warning: format not a string literal and no
>> format arguments
>>
>> These happen where we use code constructs such as:
>> SVN_ERR(svn_cmdline_printf(pool, header->data));
>>
>> The concern here is that the variable could be untrusted and this could have
>> security implications. The "proper" way to do this is:
>> SVN_ERR(svn_cmdline_printf(pool, "%s", header->data));
>>
>> Now, in lots of cases the string we're directly printing is completely
>> internally generated, so this is just an extra step (albeit one which silences a
>> warning). In other cases, this may be legitimately required. However, instead
>> of taking the time to audit the code and make the distinction, and for
>> consistency's sake, we may just want change all these calls to use an explicit
>> format string.
>>
>> Thoughts?
>>
>> -Hyrum
>>
>>
> When I've turned up the error reporting, I've seen those warnings,
> too. It also happens when we do something like:
>
> printf(timedate_format, arg1, arg2);
>
> You can't do much about those. But for constructs like you pointed
> out: definitely, let's add the "%s". It is much safer to use that over
> the lifetime of the code. Who knows where the argument might come from
> next week.
>
> Cheers,
> -g

But there's quite a number of these pointed out by Greg. That is, where the
format string is provided in a const char* to be able to select one from a
known set.

When I look at the first one I'm seeing:

(Follow use of the argument MSG and a caller right below)
subversion/libsvn_subr/io.c:2644
[[[
static svn_error_t *
do_io_file_wrapper_cleanup(apr_file_t *file, apr_status_t status,
                           const char *msg, const char *msg_no_name,
                           apr_pool_t *pool)
{
  const char *name;
  svn_error_t *err;

  if (! status)
    return SVN_NO_ERROR;

  err = file_name_get(&name, file, pool);
  if (err)
    name = NULL;
  svn_error_clear(err);

  if (name)
    return svn_error_wrap_apr(status, _(msg),
                              svn_path_local_style(name, pool));
  else
    return svn_error_wrap_apr(status, _(msg_no_name));
}

svn_error_t *
svn_io_file_close(apr_file_t *file, apr_pool_t *pool)
{
  return do_io_file_wrapper_cleanup
    (file, apr_file_close(file),
     N_("Can't close file '%s'"),
     N_("Can't close stream"),
      pool);
}
]]]

It's not really idealistic code, but it's local to that file and a helpful
and desirable code clarification. It could be mended, but why? There are
also other cases where format strings are stored in an array. Mending all of
them to get rid of the warnings wouldn't look nice. (Wrapping string
construction into functions and passing forward function pointers or
something, ugh.)

Or, these warnings will just stay there forever ... humbly -1.
I guess we should just disable this particular warning by default.

~Neels

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=982642