[Toybox] [PATCH] Fix -Wformat-security errors.

[Rob Landley]

On 01/04/2016 04:34 PM, enh wrote:
> diff --git a/lib/xwrap.c b/lib/xwrap.c
> index 6d0c511..d58ac84 100644
> --- a/lib/xwrap.c
> +++ b/lib/xwrap.c
> @@ -214,7 +214,7 @@ pid_t xpopen_both(char **argv, int *pipes)
>        // setting high bit of argv[0][0] to let new process know
>        **toys.argv |= 0x80;
>        execv(s, toys.argv);
> -      perror_msg(s);
> +      perror_msg("%s", s);

The value of s is /proc/self/exe set 6 lines earlier, it's a constant.

>        _exit(127);
>      }
> diff --git a/toys/other/bzcat.c b/toys/other/bzcat.c
> index 850c51c..93a50ac 100644
> --- a/toys/other/bzcat.c
> +++ b/toys/other/bzcat.c
> @@ -663,7 +663,7 @@ static void do_bzcat(int fd, char *name)
>  {
>    char *err = bunzipStream(fd, 1);
>  
> -  if (err) error_exit(err);
> +  if (err) error_exit("%s", err);
>  }

The value of err is one of these values from line 644:

  char *bunzip_errors[] = {0, "not bzip", "bad data", "old format"};

I.E. still not user-supplied content.

>  void bzcat_main(void)
> @@ -700,7 +700,7 @@ static void do_bunzip2(int fd, char *name)
>    if (toys.optflags&FLAG_v) {
>      printf("%s\n", err ? err : "ok");
>      toys.exitval |= !!err;
> -  } else if (err) error_msg(err);
> +  } else if (err) error_msg("%s", err);

Same.

>    // can't test outfd==1 because may have been called with stdin+stdout closed
>    if (rename) {
> diff --git a/toys/other/hexedit.c b/toys/other/hexedit.c
> index b3bde2e..c45ef1c 100644
> --- a/toys/other/hexedit.c
> +++ b/toys/other/hexedit.c
> @@ -173,7 +173,7 @@ void hexedit_main(void)
>  
>      // Display cursor and flush output
>      highlight(x, y, ro ? 3 : side);
> -    xprintf("");
> +    xflush();

Ok, sure. :)

>      // Wait for next key
>      key = scan_key(keybuf, 1);
> diff --git a/toys/other/stat.c b/toys/other/stat.c
> index fe522b0..ecaa486 100644
> --- a/toys/other/stat.c
> +++ b/toys/other/stat.c
> @@ -129,7 +129,7 @@ static void print_statfs(char type) {
>  
>      for (i=0; i<ARRAY_LEN(nn); i++)
>        if (nn[i].num == statfs->f_type) s = nn[i].name;
> -    xprintf(s);
> +    xprintf("%s", s);
>    } else if (type == 'i')
>      xprintf("%08x%08x", statfs->f_fsid.__val[0], statfs->f_fsid.__val[1]);
>    else if (type == 's') xprintf("%d", statfs->f_frsize);

Also a case where we're outputting a locally defined string that we know
has no escapes (here the result of a table lookup or the default
"unknown"), not user supplied content.

So all of these "fix" false positives generated by the security checks,
not one is an actual problem in the code. (Although one did save an
argument.)

Lemme think about this. Possibly I need to split CONFIG_TOYBOX_DEBUG
into multiple symbols, or maybe I need to add a perror_exit_nofortify()
(with a better name), maybe add -Wno-error=format-security to the cflags
to switch the breakage back _off_ since it _is_ false positives...

How is the -Werror=format-security added? (If I switch it off in the
cflags does it get added back on at the end or at the beginning of the
command line? Presumably by the specfile...)

Sigh, I need to set up a new android build environment to try this out...

Rob