4 ms·

I've had a quick look through and I really like this. Bookmarked for future reading. As a security person, I have just one request - in 2.9 you explain/recall

I've had a quick look through and I really like this. Bookmarked for future reading. As a security person, I have just one request - in 2.9 you explain/recall how functions look in C, then you explain printf, then the first C statement you execute is printf("Enter an unsigned decimal integer: "); Please, please can this kind of thing be printf("%s", "Enter an unsigned decimal integer: "); 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. In my opinion, a printf with only one argument is always a code smell. EDIT: also, the solutions to exercises 1 and 2 in 2.16 are vulnerable to buffer overflows.

wolfgke 9y ago
> 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. as cargo-cult coding.
- red_admiral 9y ago
  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.
  - boomlinde 9y ago
    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".
    - wolfgke 9y ago
      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.
      - boomlinde 9y ago
        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 :)
unwind 9y ago
Even better would of course be puts(string); -- no point in running a parser that will scan each character looking for formatting instructions, when you don't have any. Using puts() will give you a terminating line feed though, which might not be what you want for a prompt. On the other hand that likely saves you from having to deal with output-flushing and so on ... Might be nice for teaching.
- red_admiral 9y ago
  fputs(stdout, string) avoids the newline too, although you'd now have to teach two functions (printf, fputs) as well as saying what "stdout" means. In a teaching context, printf("%s", string) is probably best as printf is being used in other lines in the same file already.
  - boomlinde 9y ago
    What sacrifices you are willing to make in a teaching context is a matter of taste. You draw the line somewhere between using printf in an entirely acceptable way where fputs would have been better suited, and using printf in another entirely acceptable way where fputs would have been better suited. That seems inconsistent and hard to justify.

wolfgke 9y ago

> 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. as cargo-cult coding.

red_admiral 9y ago
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.
- boomlinde 9y ago
  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".
  - wolfgke 9y ago
    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.
    - boomlinde 9y ago
      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 :)

unwind 9y ago

Even better would of course be puts(string); -- no point in running a parser that will scan each character looking for formatting instructions, when you don't have any. Using puts() will give you a terminating line feed though, which might not be what you want for a prompt. On the other hand that likely saves you from having to deal with output-flushing and so on ... Might be nice for teaching.

red_admiral 9y ago
fputs(stdout, string) avoids the newline too, although you'd now have to teach two functions (printf, fputs) as well as saying what "stdout" means. In a teaching context, printf("%s", string) is probably best as printf is being used in other lines in the same file already.
- boomlinde 9y ago
  What sacrifices you are willing to make in a teaching context is a matter of taste. You draw the line somewhere between using printf in an entirely acceptable way where fputs would have been better suited, and using printf in another entirely acceptable way where fputs would have been better suited. That seems inconsistent and hard to justify.