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

[enh]

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

not for functions such as fprintf where the c library is providing the
annotation, but, yes, if you have your own printf-like functions and
you don't annotate them, clang can't infer that it's printf-like.

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

yeah, but we'd still turn format string checking on. "make silly
mistakes harder to make" is SOP.

to me the reason to break it up is that it's not really about
debugging. there's "DO_NOT_READ_UNINITIALIZED_MEMORY" and
"WARN_ABOUT_FORMAT_STRING_ERRORS" and a handful of extra error checks.

> But I'm leaning towards perror_msg1() as the right fix for most of this.

makes sense. let me know if you'd like a patch.

> (Probably in the morning, it's after midnight here...)
>
> Rob



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