Hacker News Reader

Comment by apaprocki | Hacker News Reader

Worth noting that static analysis finds bugs like this immediately. TLS code seems like the perfect candidate to run through static analysis on every checkin. There are products such as Coverity and PVS-Studio that would have immediately flagged this and probably some open-source ones built around LLVM as well (unsure about this one, though). I personally use Coverity and have it hooked up in the same way everyone connects Travis CI.

View on HN

> Worth noting that static analysis finds bugs like this immediately.

I'd like to point out the case where Debian maintainers "fixed" a "bug" discovered by static analysis; https://blog.isotoma.com/2008/05/debians-openssl-disaster/.

Valgrind is dynamic analysis, not static analysis.

There is no excuse to bypass static analysis nowadays. At very least should be part of the continuous integration build.

The problem are the many developers that still think they are perfect and know the full C standard, including undefined parts.

That's a rare problem among developers. The more common problem is that developers make mistakes. All of them.

Static analysis, regression tests and turning on all the warnings you possibly can should be mandatory, especially for such critical pieces of code.

> That's a rare problem among developers.

Many of the C guys I met along my career thought otherwise.

> Static analysis, regression tests and turning on all the warnings you possibly can should be mandatory, especially for such critical pieces of code.

And if you are going to disable a specific warning, it should only be for a limited section of code and include a detailed comment about why the warning is superfluous and why the code is safe.

Unfortunately the decision to use static analysis tools would have to come from developers who are comfortable admitting they make mistakes sometimes.

It takes a special kind of ego to write an SSL library with no unit tests, not turn on compiler warnings, and not use static analysis tools.

Fully agree, I met quite a few of those egos already.

OpenSSL was written by monkeys: http://www.peereboom.us/assl/assl/html/openssl.html

Very enlightening.

Wow. Thats really bad, who would have thought OpenSSL is such a piece of crap? Its about time someone writes a decent alternative!

5 lines of a unit test also verify this:

https://github.com/landonf/Testability-CVE-2014-1266/blob/ma...

Yep, PC-lint from Gimpel catches it:

Error -> Warning 527 Unreachable code at token 'ret' (col 12)

Nice. Does PC-lint give a lot of false negatives and positives as well?

If the code hasn't been linted, expect lots of false positives. I usually have to disable entire classes of less serious warnings to notice the errors, then slowly work through to the less serious stuff.

Regarding Adam Langley's comment about -Wall in gcc: -Wextra is what you want to have many more tests analysing your code. I wonder if this would have caught it.

A quick test shows neither -Wall nor -Wextra report anything on either gcc or clang. However, clang's -Weverything does complain (it implies -Wunreachable-code).

Confusingly, gcc accepts -Wunreachable-code as a valid option, but then proceeds not to warn anything. (edit): Which is a known bug apparently,

http://gcc.gnu.org/ml/gcc-help/2011-05/msg00360.html

My personal observation is that while compilers continually get better at identifying straightforward mistakes, they don't have the same capability as a static analysis tool that works across compilation units. That is the real selling point of these tools. Definitely -Weverything/-Wexta plus static analysis for baseline checking.

My other beef is that compilers always add new warnings as options or behind new "catch all" flags like -Weverything that no one knows about. As long as each new warning can be individually disabled, there isn't a huge cost to pay by making much more of them enabled by default. Upgrading to a new compiler version usually requires a tiny bit of work, so adding a few -Wno-* rules for new things you want to disable until the code is clean (or forever) is a small price to pay for all new code getting the checks by default.

The real WTF is those Warn flags, then. In what sane world does "extra" mean more than "all", and "all" and "everything" mean different things?

Apple introduced -Weverything because suddenly changing the scope of gcc's -Wall (which indeed does not include everything) would break a lot of builds.

"suddenly" is a funny way to say "over a span of years"....

Is "-Weverything" inclusive of new warnings? Because they're just kicking the can if not. What's next, "-Wreallyeverything"?

Yep. Even ones that may not be fully baked, so some people caution against it.

We have it turned on, and about 10 warnings specifically disabled because they were too noisy or not useful for us. It's always interesting upgrading Xcode and seeing what new warnings we get to fix.

Excessive fear of backwards compatibility: you make -Wall. People use it. You add a new check which will reject some shoddy code. “What if people complain that their project no longer builds with -Wall? Let's add a new setting…”. Repeat…

I've been wondering how hard it would be to get to the point where the defaults are rigorous and developers have to opt out with specific -Wno-… options.

"-Wall" actually means, "all warnings, as of the date we froze the definition of -Wall, which for all you know was 20 years ago, good luck."

Microsoft VC++ has a -Wall warning level too, and it really does mean all warnings. Each major version of VC++ potentially introduces new warnings to -Wall.

PVS-Studio might be a bit unlikely however in this particular case.

Reply on news.ycombinator.com