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
> > <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.
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
Received on 2008-12-11 03:27:59 CET