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

[enh]

On Mon, Jan 4, 2016 at 6:57 PM, Rob Landley <rob at landley.net> wrote:
> 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.

yes, but it's easier to fix toybox than it is to fix clang.

>>        _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.)

well, that's in part because somehow my patch is missing the bug fix in shred.c:

--- a/toys/other/shred.c
+++ b/toys/other/shred.c
@@ -97,7 +97,7 @@ void shred_main(void)
         if (len-pos < throw) throw = len-pos;

       if (iter != TT.iterations) xread(TT.ufd, toybuf, throw);
-      if (throw != writeall(fd, toybuf, throw)) perror_msg("%s");
+      if (throw != writeall(fd, toybuf, throw)) perror_msg("%s", *try);
       pos += throw;
     }
     if (toys.optflags & FLAG_u)

> Lemme think about this. Possibly I need to split CONFIG_TOYBOX_DEBUG
> into multiple symbols,

that sounds like a good idea anyway.

> 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...

no, this is silly. you do make mistakes with format strings. each time
i've done this sweep there have been real mistakes (even though i
managed to not include it in the patch this time), and we're none of
us ever going to be as vigilant as a compiler. it's silly not to take
advantage of this because the cost of making the compiler happy in the
false positive cases is so small.

if you _do_ want to add a new function, add one that takes a string
literal instead of a format string and arguments. then you can keep
the tricksy style without confusing the compiler or readers.

> 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...)

it's set globally, and if you do find a way to turn it off, we'd
consider that a bug and fix it.

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



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.