You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Neels Janosch Hofmeyr <ne...@elego.de> on 2008/12/11 02:16:36 UTC
Re: "format not a string literal" warnings
Greg Stein wrote:
> On Mon, Nov 10, 2008 at 5:36 AM, Hyrum K. Wright
> <hy...@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
Re: "format not a string literal" warnings
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2008-12-11 at 03:16 +0100, Neels Janosch Hofmeyr wrote:
> Greg Stein wrote:
> > On Mon, Nov 10, 2008 at 5:36 AM, Hyrum K. Wright
> > <hy...@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.
I think there are two similar types of warning being produced. In the
quotes above, Hyrum and Greg were talking about fixing the warnings of
type "format not a string literal and no format arguments". The one you
pointed out was of the type "format not a string literal so arguments
can't be checked". I agree there's probably no good way to fix those so
we probably should disable the warning after fixing all of the first
kind.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=982645