5 ms·

I wrote a cryptographic library in C. The quality requirements for that kind of thing is pretty much "don't publish a single error, or Thomas Ptacek is going to

I wrote a cryptographic library in C. The quality requirements for that kind of thing is pretty much "don't publish a single error, or Thomas Ptacek is going to haunt you for the rest of your life[1]". Or, you know, actual consequences that might arise from users using your broken crypto. Inevitably, I did publish some errors, one of which was actually critical. Here's how many would have been caught by a compiler warning not already present in GCC-8's -Wall -Wextra: Zero. None. Nada. Zilch. Mind you, I did have reports of some other compiler (mostly MSVC, but Clang with -Weverything as well) complaining about various weird things, including stuff like using bitwise operations on signed integers, or negation on an unsigned integer. They never lead to the discovery of an actual bug. On the other hand, one very important thing I did was using sanitizers extensively. ASan, MSan, UBSan, Valgrind, even the TIS interpreter (and now TIS CI), so I could weed out as much undefined behaviour as I could. And boy did those tools find actual bugs. Those, coupled with a proper test suite, will find all the bugs. The bugs I failed to find in time can all be traced with either not using the strictest runtime sanitiser, or a hole in the test suite. That being said, I still try very hard to be warning-free for most compilers out there. And I mostly am. But this is not about bugs. This is about sparing my users the headache that comes with investigating some trivial warning they should not care about. Monocypher does not need -Werror. [1]: I'm joking, he actually didn't.

UncleMeat 5y ago
All advice has exceptions. But I don't think it would be controversial to say that in most engineering organizations, -Werror will catch and prevent real bugs at a nontrivial rate.
- loup-vaillant 5y ago
  I strongly suspect the reason is because they tolerate incomplete test suite, and don't use sanitisers enough, despite their use of an insanely unsafe language (one thing I absolutely hate about C is the sheer amount of undefined behaviour). -Werror is just a stop gap desperate measure for when devs are pressured to write crap as quickly as possible. What's exceptional about Monocypher is not really that it's a cryptographic library (though that helps, modern cryptographic code tends to have pathologically simple control flow), it's that I took the time to polish it. That being said, I am not against static analysis. On the contrary, I am huge fan of strong static type systems, and for languages like C a static analyser can be a huge help. I also trust John Carmack when he says running a static analyser has found non-trivial bugs in real code. But then I want real static analysis, that can assess actual risks of making mistakes. Compilers need to be fast, so their warnings naturally tend to be crude. And if they start complaining about stuff like whitespace in a language where whitespace has no semantics, that may hurt more than it helps.
  - UncleMeat 5y ago
    "Just write good code" is not a workable plan for most organizations of any size. You often want workflows that are resistant to "we promoted somebody to TL when they shouldn't have been and they are not doing a very good job" situations. Yes, it is outrageous that anybody still writes programs in C or C++, which basically guarantee that you'll write oodles of subtle bugs. Yes, static analysis that finds problems that you can't train humans to find is more interesting. Yes, sanitizers and fuzzing should be mandatory for any serious project in C or C++. But "hey, you've got a vacuously true branch predicate" is valuable in a large number of cases even if humans can catch that with tests, since most organizations are not able to successfully hire only excellent engineers and give them sufficient time to never be rushed. Just a few weeks ago Google shipped a branch condition that included & rather than && that broke all Chromebook logins. I submit this as evidence that it it not possible to construct a large organization that is resistant to bad code without automation. Yes, a lot of other things needed to go wrong to enable this code to reach production but so many of those processes are either done by humans (and therefore error prone) or produce the same complaints when mandated by automation (e.g., 100% line coverage).
    - MaxBarraclough 5y ago
      > Google shipped a branch condition that included & rather than && that broke all Chromebook logins To be fair to C++, that's not the sort of issue that can really be blamed on the language, its unsafety, or its many footguns. Did they skimp on testing?
      - UncleMeat 5y ago
        I don't know the testing situation. But there is a lesson from other forms of engineering here. Almost no failure has a single cause. A variety of things need to go wrong. "Just test better" is not a strategy. Any honest view of large scale software engineering has to internalize that all of the human processes can fail and even the technical processes can fail and so you need layers of protection. People should try to write correct code. Sometimes they will fail. People should review code carefully. Sometimes they will fail. People should test code thoroughly. Sometimes they will fail. Tooling should automate issue detection with static analysis and fuzzing. Sometimes it will fail. Tooling should automate issue detection with canaries and monitoring. Sometimes it will fail. Maybe the team skimped on testing as a rule. Maybe the engineers who submitted and review the code were tired that day and didn't test it well. Maybe something in the the CI was flakey so they did a force push. I don't know. But I do know that making it harder to do weird things like using bitwise-and in a branch predicate with something other than a constant mask adds an extra layer of defense.
bjourne 5y ago
No, it is rare for compiler warnings to be about important errors. But once in a blue moon they are, so why not be cautious? While your library is fairly small (only 3k lines), so I don't know what conclusions can be drawn from it, I checked out a version of it from 2017. Compiling it with gcc 11 generates only two warnings: x25519.c:9:22: warning: '_0' defined but not used [-Wunused-const-variable=] 9 | static const uint8_t _0[16]; | ^~ ed25519.c:302:14: warning: argument 1 of type 'u8[64]' {aka 'unsigned char[64]'} with mismatched bound [-Warray-parameter=] 302 | void HASH(u8 hash[64], const u8 in, size_t inlen); | ~~~^~~~~~~~ In file included from ed25519.c:285: sha512.h:19:29: note: previously declared as 'uint8_t ' {aka 'unsigned char '} 19 | void crypto_sha512(uint8_t out,const uint8_t *input, size_t input_size); Thus it appears to me that both the benefit and the cost of -Werror for your library is small.
- deleted 5y ago
  [deleted]
- loup-vaillant 5y ago
  > But once in a blue moon they are, so why not be cautious? You have to balance the inconvenience of false positives against the consequences of false negatives. If the false negatives are rare enough, compared to the false positives, it might be best not to be cautious. Still, in practice I am fairly cautious, and tolerate zero warning. When someone reports a warning to me, I always try to fix it. So in practice, what I do is fairly close to -Werror, with one big difference: if some compiler comes up with some new warning, or if a set of compilers are impossible to please at the same time, my users aren't blocked. Which is important, because new warnings are now overwhelmingly likely to be false positives. That mismatch bound you just showed however does set off my alarm bells. I'll need to make sure I actually got rid of it.

UncleMeat 5y ago

All advice has exceptions. But I don't think it would be controversial to say that in most engineering organizations, -Werror will catch and prevent real bugs at a nontrivial rate.

loup-vaillant 5y ago
I strongly suspect the reason is because they tolerate incomplete test suite, and don't use sanitisers enough, despite their use of an insanely unsafe language (one thing I absolutely hate about C is the sheer amount of undefined behaviour). -Werror is just a stop gap desperate measure for when devs are pressured to write crap as quickly as possible. What's exceptional about Monocypher is not really that it's a cryptographic library (though that helps, modern cryptographic code tends to have pathologically simple control flow), it's that I took the time to polish it. That being said, I am not against static analysis. On the contrary, I am huge fan of strong static type systems, and for languages like C a static analyser can be a huge help. I also trust John Carmack when he says running a static analyser has found non-trivial bugs in real code. But then I want real static analysis, that can assess actual risks of making mistakes. Compilers need to be fast, so their warnings naturally tend to be crude. And if they start complaining about stuff like whitespace in a language where whitespace has no semantics, that may hurt more than it helps.
- UncleMeat 5y ago
  "Just write good code" is not a workable plan for most organizations of any size. You often want workflows that are resistant to "we promoted somebody to TL when they shouldn't have been and they are not doing a very good job" situations. Yes, it is outrageous that anybody still writes programs in C or C++, which basically guarantee that you'll write oodles of subtle bugs. Yes, static analysis that finds problems that you can't train humans to find is more interesting. Yes, sanitizers and fuzzing should be mandatory for any serious project in C or C++. But "hey, you've got a vacuously true branch predicate" is valuable in a large number of cases even if humans can catch that with tests, since most organizations are not able to successfully hire only excellent engineers and give them sufficient time to never be rushed. Just a few weeks ago Google shipped a branch condition that included & rather than && that broke all Chromebook logins. I submit this as evidence that it it not possible to construct a large organization that is resistant to bad code without automation. Yes, a lot of other things needed to go wrong to enable this code to reach production but so many of those processes are either done by humans (and therefore error prone) or produce the same complaints when mandated by automation (e.g., 100% line coverage).
  - MaxBarraclough 5y ago
    > Google shipped a branch condition that included & rather than && that broke all Chromebook logins To be fair to C++, that's not the sort of issue that can really be blamed on the language, its unsafety, or its many footguns. Did they skimp on testing?
    - UncleMeat 5y ago
      I don't know the testing situation. But there is a lesson from other forms of engineering here. Almost no failure has a single cause. A variety of things need to go wrong. "Just test better" is not a strategy. Any honest view of large scale software engineering has to internalize that all of the human processes can fail and even the technical processes can fail and so you need layers of protection. People should try to write correct code. Sometimes they will fail. People should review code carefully. Sometimes they will fail. People should test code thoroughly. Sometimes they will fail. Tooling should automate issue detection with static analysis and fuzzing. Sometimes it will fail. Tooling should automate issue detection with canaries and monitoring. Sometimes it will fail. Maybe the team skimped on testing as a rule. Maybe the engineers who submitted and review the code were tired that day and didn't test it well. Maybe something in the the CI was flakey so they did a force push. I don't know. But I do know that making it harder to do weird things like using bitwise-and in a branch predicate with something other than a constant mask adds an extra layer of defense.

bjourne 5y ago

No, it is rare for compiler warnings to be about important errors. But once in a blue moon they are, so why not be cautious? While your library is fairly small (only 3k lines), so I don't know what conclusions can be drawn from it, I checked out a version of it from 2017. Compiling it with gcc 11 generates only two warnings: x25519.c:9:22: warning: '_0' defined but not used [-Wunused-const-variable=] 9 | static const uint8_t _0[16]; | ^~ ed25519.c:302:14: warning: argument 1 of type 'u8[64]' {aka 'unsigned char[64]'} with mismatched bound [-Warray-parameter=] 302 | void HASH(u8 hash[64], const u8 in, size_t inlen); | ~~~^~~~~~~~ In file included from ed25519.c:285: sha512.h:19:29: note: previously declared as 'uint8_t ' {aka 'unsigned char '} 19 | void crypto_sha512(uint8_t out,const uint8_t *input, size_t input_size); Thus it appears to me that both the benefit and the cost of -Werror for your library is small.

deleted 5y ago
[deleted]
loup-vaillant 5y ago
> But once in a blue moon they are, so why not be cautious? You have to balance the inconvenience of false positives against the consequences of false negatives. If the false negatives are rare enough, compared to the false positives, it might be best not to be cautious. Still, in practice I am fairly cautious, and tolerate zero warning. When someone reports a warning to me, I always try to fix it. So in practice, what I do is fairly close to -Werror, with one big difference: if some compiler comes up with some new warning, or if a set of compilers are impossible to please at the same time, my users aren't blocked. Which is important, because new warnings are now overwhelmingly likely to be false positives. That mismatch bound you just showed however does set off my alarm bells. I'll need to make sure I actually got rid of it.