Re: sharutils does not build with -Werror=format-security

[Bruce Korb]

On 10/12/13 08:21, Santiago Vila wrote:
> Hello.
>
> The following patch is required to allow compilation of
> sharutils 4.13.5 with -Werror=format-security.
>
> Thanks.

Hi Santiago, Simon, et al.

OK.  I give:  what possible good is -Werror=format-security?
According to the doc, it is dangerous to use variables as formats
because the variable "just might" contain a %n and wreak havoc.
That makes i18n an interesting challenge.  If the format contains
translatable text, the formatting string *MUST* not be static.
In other words, code like this:

> static int
> check_accessibility (const char *local_name, const char *restore_name)
> {
>   if (access (local_name, 4))
>     {
>       error (0, errno, _("Cannot access %s"), local_name);
>       return SHAR_EXIT_FILE_NOT_FOUND;
>     }

must be left alone.  If that has to be left alone, then:

> @@ -954,7 +954,7 @@
>           free (c_dir);
>         }
>       else
> -      error (0, errno, _("Cannot get current directory name"));
> +      error (0, errno, "%s", _("Cannot get current directory name"));
>     }
>   }

this change is pointless nonsense.  I think GCC went off the rails when
they warned "OMG! You have a NUL byte in the format string" and this
is again an overkill.  If someone switches out the l10n library for
a bogus one, then you get the old garbage-in-garbage-out problem.

Where the third argument to error is, in fact, some variable string,
I will pass it through a "%s" format specifier.  I suspect they should
all be validated as clean, but that patch is easier than verifying
validity.  Compile with this warning set IFF NLS is disabled.
If Debian/GCC still complains, then the warning should be silenced.

> diff --git a/src/shar.c b/src/shar.c
> index f12cf19..3ce860c 100644
> --- a/src/shar.c
> +++ b/src/shar.c
> @@ -444,7 +444,7 @@ walkdown (
>
>    if (stat (local_name, &struct_stat))
>      {
> -      error (0, errno, local_name);
> +      error (0, errno, "%s", local_name);
>        return SHAR_EXIT_FILE_NOT_FOUND;
>      }
>
> @@ -453,7 +453,7 @@ walkdown (
>
>    if (directory = opendir (local_name), !directory)
>      {
> -      error (0, errno, local_name);
> +      error (0, errno, "%s", local_name);
>        return SHAR_EXIT_CANNOT_OPENDIR;
>      }
>
> @@ -535,7 +535,7 @@ walkdown (
>  #else
>    if (closedir (directory))
>      {
> -      error (0, errno, local_name);
> +      error (0, errno, "%s", local_name);
>        return SHAR_EXIT_CANNOT_OPENDIR;
>      }
>  #endif
> @@ -587,7 +587,7 @@ walktree (walker_t routine, const char *local_name)
>
>
>      if (status != 0)
>        {
> -        error (0, errno, local_name_copy);
> +        error (0, errno, "%s", local_name_copy);
>          status = SHAR_EXIT_FILE_NOT_FOUND;
>        }
>      else
> @@ -2368,7 +2368,7 @@ main (int argc, char ** argv)
>                optionLoadLine (&sharOptions, arg);
>              }
>            else
> -            error (0, errno, arg);
> +            error (0, errno, "%s", arg);
>            continue;
>          }