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

[Rob Landley]

On 01/04/2016 09:17 PM, enh wrote:
> 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.

Yes, but I'm not sure this is the right fix.

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

Ah. That one I screwed up in the _other_ direction. :)

>> Lemme think about this. Possibly I need to split CONFIG_TOYBOX_DEBUG
>> into multiple symbols,
> 
> that sounds like a good idea anyway.

At some point it gets big enough to need extra granularity, but having
too many config options is a different kind of cost...

(It's always the little things that screw me up. The big problems tend
to have big obvious solutions. The little problems have little fixes
that aren't a big enough win to be compelling, so I waffle back and
forth about which way to go. It's really annoying. Probably on your end
too. Sorry about that.)

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

Indeed. That's why the warning ability is still there. It's just the
-Werror part that's causing a problem.

I agree that eliminating the warnings is good, which means marking the
ones that aren't a real problem. The question is how.

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

Agreed. But the compiler's extra syntax checking mode is also producing
false positives that cause us to bloat the code to shut it up, and that
makes me wince. And it's not just one or two instances, the gratuitous
extra argument is being repeated a _lot_.

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

The cost of each individual cases is small, yes. But the add up, and the
code already has a bunch of instances of this extra argument:

  $ grep 'error_msg("%s"' lib/*.c toys/*/*.c *.c | wc -l
  33

Which is getting to the point where it's probably worth adding
error_msg1(string) and perror_msg1(string) versions that only take one
argument (basically a printf("%s", arg) wrapper) so all the callers
don't have to do it over and over).

I didn't do that before because having lots of library functions that do
_almost_ the same thing is its own cost, but 33 instances plus another
half-dozen here, several of which are otherwise completely useless
except to shut up false positives in a static checker? Probably worth it
at this point.

(Hence the "I need to think about this a bit". More than one way to fix
the problem and it wasn't immediately obvious to me what the best fix was.)

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

Exactly, yes. Which lets me remove 33 _existing_ "%s" arguments, plus
avoid adding more in this patch, so probably a net win despite the extra
library functions.

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

Well, it _is_ turned off when you don't enable the DEBUG config option.
And if I did the "split up DEBUG", fix this -Werror would get its own
option so you could still turn it off. (Not enabling it == turning it off.)

But I'm leaning towards perror_msg1() as the right fix for most of this.
(Probably in the morning, it's after midnight here...)

Rob