I think I may have misunderstood the original suggestion. I was under
the impression that printf(fmt); was error-worthy, not printf(fmt,
params);. I agree with David that turning the latter into an error
would break reasonable code.
~Aaron
Note from the original post:
“This warning complains about a printf-like format string that is not a literal string and is used without any arguments.”
That means that ‘printf(translate(“Please press OK to continue”));’ would trigger this warning (rightfully). But the example you gave would not trigger the warning, as the invocation has extra ‘lo’ and ‘hi’ arguments.
_______________________________________________ cfe-dev mailing list [cfe-dev@lists.llvm.org](mailto:cfe-dev@lists.llvm.org) [http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev](http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev)
I think the recent bug I opened up is pertinent, as it’s not clearly (for some people, like me) bad code at fault:
Let me attempt a summary here:
- For the specific warning, it sounds like making it more useful (give it a fixit, improve error text – see the bug that was just linked to, etc) will likely make people pay more attention to it
- It sounds like a “warning level” concept is considered useful by some since there’s too much stuff on by default (?) and the jump from “default warnings” to “-Wall” is too big. From what I gathered, people want something more granular than default, -wall, -wextra, but something less granular than turning individual groups on and off.
- Some people think it might make sense to turn on warnings-as-errors for lower warning levels.
- But that’s contested, and it’s also not clear if -Wformat-security would be in a low warning level category.
So I think the consensus to the question in the original post is what Reid said (“My feeling is that we shouldn’t do this in upstream clang. […] think it’s totally reasonable for vendors to want to make this default to an error, though”).
While the case of argument-less format strings is quite likely to be an
error, the slightly more generic case of non-literal format string with
arguments (or va_list) does introduce a non-trivial number of false
positives. As there is no workaround not involving compiler-specific
features, that is completely unacceptable as default error.
In my experience, even the case of argument-less format strings has a
significant FP rate, so this is even borderline for default-on warning.
The only justification for that is IMO the presence of a standard
compliant workaround, incidently often resulting in better code.
Joerg
I agree. This is not an error, just bad/dangerous programming.
We could even put it on by default but not make it into an error.
cheers,
--renato
>>
>> Sorry, but printf(fmt); is *always* a true positive in my book. Same
>> with failing to return from all code paths. (etc)
>
> You are wrong. The most common reason for printf(fmt) to appear is
that fmt is the result of doing a lookup of the locale-aware version of
some constant string. In this case, the contents of fmt is entirely under
the control of whoever shipped the application, and will have been checked
for format string vulnerabilities by the localisation tools (at least,
assuming that the original that is being translated are free from
vulnerabilities). If you are not doing any caching in the application,
then you can mark the translation function with the attribute that
indicates that its input and output have the same format string
compatibility. If you are caching, then there is no easy way of silencing
this warning.
>
> Making this an error will cause valid and correct code to fail to
compile and will result in people simply disabling the warning, rather than
checking it.If the expected string does not have any format specifiers, then
printf("%s", fmt) is definitely the correct way to write that because
the assumption "entirely under the control of whoever shipped the
application" is a poor one. If it does have format specifiers, I agree
that we should not err, but I don't believe that was on the table.I think David is talking about a situation where it is e.g.
printf(translate("Please enter a number from %d-%d\n"), lo, hi);
Note from the original post:
"This warning complains about a printf-like format string that is not
a literal string and is used without any arguments."
That means that 'printf(translate("Please press OK to continue"));' would
trigger this warning (rightfully). But the example you gave would not
trigger the warning, as the invocation has extra 'lo' and 'hi' arguments.
Even in the case of a single argument to printf, I think an idiom like
translation could apply - it would seem plausible that a translation
dictionary would have strings in a consistent format, and that format would
be printf-escaped text, rather than only having strings with placeholders
be printf-escaped. So it would make sense to always printf a translated
string, I would think.
This sounds reasonable to me. Thanks for all the feedback.
I’m testing a patch to add a fix-it and I found some other things to clean up related to this.
Another interesting case is from gnumeric
(https://github.com/paulfitz/gnumeric/blob/master/src/tools/analysis-frequency.c,
lines 144-155):
switch (info->base.group_by) {
case GROUPED_BY_ROW:
format = _("Row %d");
break;
case GROUPED_BY_COL:
format = _("Column %d");
break;
default:
format = _("Area %d");
break;
}
dao_set_cell_printf (dao, col, 1, format, col);
("dao_set_cell_print" is just a wrapper around printf)
But we can't compile this code already! -- as gnumeric build adds
"-Werror=format=2" that turns "format string is not a string literal"
warning into an error:
analysis-frequency.c:155:38: error: format string is not a string literal
[-Werror,-Wformat-nonliteral]
dao_set_cell_printf (dao, col, 1, format, col);
^~~~~~
Yours,
Andrey
This is a completely different issue than what was discussed earlier.
First of all, it is using non-standard flags. Second, it can be easily
rewritten into a form that is safte and doesn't trigger the warning by
pushing the dao_set_cell_printf into the switch. Note that gettext
itself is annotated for format string compatibility, so it doesn't count
as non-literal by itself.
Joerg