> I know this string is a compile-time constant, so it's not going to hurt anyone, but you're teaching people the old myth that printf(string) is the way to print a string and at some point they'll try this with a user-supplied string.
I am also very security-conscious in my programming, but the rule that I apply is
printf(string);
is the way to use if string is some compile-time constant that visibly contains no '%' character and
printf("%s", string);
is the way to go otherwise. In this sense I consider the rule
> In my opinion, a printf with only one argument is always a code smell.
Your answer is correct, secure and has fewer false positives than mine. But I interpret cargo cult to mean doing something with no unserstanding why it should work (and in the original example, ending up with "it doesn't work"). That's not the case here.
"Code smell" to me means code that should make you feel bad when reading/writing it, as opposed to code that's wrong - a psychological device to make code that often is wrong, always look wrong. The classic example is if(a = b) which is sometimes really what you want - but I'd write this if((a = b)) in this case to make it clear it's deliberate. printf("%s", str) is the same kind of thing, and it means when I'm reviewing code I don't have to go and check whether str really is a compile-time constant or not. Better still, it's also a sign to the human reader that there's no fancy formatting going on here - you're really just printing a string.
"Always use 2+ arguments to printf" is a rule for humans that has the advantage of being really easy to follow and to teach and avoids the most common version of the vulnerability. I don't mind people who know what they're doing breaking this rule (sometimes breaking a rule when you know it's ok is pretty much a definition of being an expert) but I do include the rule whenever I'm teaching printf to beginners.
From a security perspective I would be more concerned with using printf at all when puts/fputs would have been fine and less susceptible to possible run-time errors. If you're not really using a formatting template for anything meaningful, you shouldn't be using a string formatting function. So the point that you should "always use 2+ arguments to printf" holds, but the solution isn't to make the first argument "%s".
I accept your reasoning, but I am a little bit divided on this advice. On the one hand your reasoning is sound. On the other hand, in particular in an embedded or security field one wants to keep the code footprint really small if possible (fewer lines of code mean fewer lines of code that might contain a bug - of course under the assumption that the bug rate is constant in both code bases). So if one does not even have puts/fputs in the codebase, there trivially cannot be any obscure corner case bug in the used puts/fputs implementation.
From a security standpoint, if your standard I/O implementation has a bug in its `fputs`, it is broken on such a fundamental level that you should print it, delete it and burn the printed copy. Better to find this out with something relatively trivial with a minimal surface for errors like puts than through an obscure format string.
`printf` gives the caller more opportunities to accidentally cause bugs. The difference in LOC when comparing for example FreeBSD `puts` to `printf` variants is so great that if you had any reason to doubt the implementation of the standard library you would be better off writing your own formatting functions and print everything with puts/putc because that would likely be easier to test and verify than identifying all the edge cases in `printf` flag parsing.
Personally, I think either way to do it is fine, but since we're seemingly all in the mood to be picky about code smell, security and such, I thought I'd join in. In reality the examples mentioned are not production code, and they are likely to be used with GNU libc, not Cheap Chip Factory libc 0.0.1 alpha. In most cases, the standard library is well exercised because its use is ubiquitous. If that can not be assumed, and you're somehow stuck with a broken standard library, I'd rather go ahead and verify puts, and let you dig into the `vsprintf` parser and make sure it works in all the cases it gives the caller an opportunity to create :)
> Your answer is correct, secure and has fewer false positives than mine. But I interpret cargo cult to mean doing something with no unserstanding why it should work (and in the original example, ending up with "it doesn't work"). That's not the case here.
To me it also includes:
- forbid patterns that are perfectly fine (as in this example), but we nevertheless forbid them for a fear of wrath of a demon, eh, I mean for the fear that it might introduce some bug (without giving a very good reasoning).
- using "best practises" (in particular "best security practises") that are either not based on strong evidence, but on some obscure oral tradition/scripture that says this is the way to go.
I am also very security-conscious in my programming, but the rule that I apply is
is the way to use if string is some compile-time constant that visibly contains no '%' character and is the way to go otherwise. In this sense I consider the rule> In my opinion, a printf with only one argument is always a code smell.
as cargo-cult coding.