Re: [Bug-tar] Format-string warnings in 1.26

[Daniel Macks]

On Thu, 28 Jul 2011 17:35:15 +0200, Joerg Schilling  wrote:
Paul Eggert <address@hidden> wrote:
> > On 07/28/11 07:44, Daniel Macks wrote:
> > >   printf(foo);
> > > > > is considered a potential security risk if foo is a variable
> rather than a simple quoted string. The solution is to do:
> > > > >   printf("%s", foo);
> >
> > I'm afraid this bug report is rather vague; without knowing the
> > details of which printf call we're talking about, there's not
> > much we can do.  Certainly there are some calls to printf-like
> > functions where the above transformation would break things,
> > as FOO is supposed to be a format.
>
> GCCs warnings in general are not well based, maybe this was a warning
> from GCC.

clang does it too (and even more verbosely). It's a valid concern when
the string being fed comes from a gettext translation (or really
anything else that derives from user-controllable data). Here's a first
pass as fixing gnulib/ and lib/ (src/ has over a hundred of them, will
take a bit to check that they really are all printf at heart).
Hopefully .patch not munged by email client...

diff -Nurd -x'*~' tar-1.26.orig/gnu/argmatch.c tar-1.26/gnu/argmatch.c
--- tar-1.26.orig/gnu/argmatch.c    2011-03-12 04:14:26.000000000 -0500
+++ tar-1.26/gnu/argmatch.c    2011-07-29 01:39:55.000000000 -0400
@@ -152,7 +152,7 @@
 
   /* We try to put synonyms on the same line.  The assumption is that
      synonyms follow each other */
-  fprintf (stderr, _("Valid arguments are:"));
+  fprintf (stderr, "%s", _("Valid arguments are:"));
   for (i = 0; arglist[i]; i++)
     if ((i == 0)
         || memcmp (last_val, vallist + valsize * i, valsize))
diff -Nurd -x'*~' tar-1.26.orig/gnu/argp-parse.c tar-1.26/gnu/argp-parse.c
--- tar-1.26.orig/gnu/argp-parse.c    2011-03-12 04:14:26.000000000 -0500
+++ tar-1.26/gnu/argp-parse.c    2011-07-29 01:42:11.000000000 -0400
@@ -156,7 +156,7 @@
       else if (argp_program_version)
         fprintf (state->out_stream, "%s\n", argp_program_version);
       else
-        __argp_error (state, dgettext (state->root_argp->argp_domain,
+        __argp_error (state, "%s", dgettext
(state->root_argp->argp_domain,
                                       
"(PROGRAM ERROR) No version known!?"));
       if (! (state->flags & ARGP_NO_EXIT))
         exit (0);
diff -Nurd -x'*~' tar-1.26.orig/gnu/openat-die.c tar-1.26/gnu/openat-die.c
--- tar-1.26.orig/gnu/openat-die.c    2011-03-12 04:14:31.000000000 -0500
+++ tar-1.26/gnu/openat-die.c    2011-07-29 01:38:35.000000000 -0400
@@ -36,7 +36,7 @@
 openat_save_fail (int errnum)
 {
 #ifndef GNULIB_LIBPOSIX
-  error (exit_failure, errnum,
+  error (exit_failure, errnum, "%s",
          _("unable to record current working directory"));
 #endif
   /* The `noreturn' attribute cannot be applied to error, since it returns
@@ -55,7 +55,7 @@
 openat_restore_fail (int errnum)
 {
 #ifndef GNULIB_LIBPOSIX
-  error (exit_failure, errnum,
+  error (exit_failure, errnum, "%s",
          _("failed to return to initial working directory"));
 #endif
 
diff -Nurd -x'*~' tar-1.26.orig/lib/rtapelib.c tar-1.26/lib/rtapelib.c
--- tar-1.26.orig/lib/rtapelib.c    2010-09-14 06:06:44.000000000 -0400
+++ tar-1.26/lib/rtapelib.c    2011-07-29 01:43:41.000000000 -0400
@@ -512,7 +512,7 @@
     /* Bad problems if we get here.  */
 
     /* In a previous version, _exit was used here instead of exit.  */
-    error (EXIT_ON_EXEC_ERROR, errno, _("Cannot execute remote shell"));
+    error (EXIT_ON_EXEC_ERROR, errno, "%s", _("Cannot execute
remote shell"));
       }
 
     /* Parent.  */

dan

 --
Daniel Macks
address@hidden