5 ms·

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. T

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.

Aissen 13y ago
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.
- throwaway_yy2Di 13y ago
  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 http://gcc.gnu.org/ml/gcc-help/2011-05/msg00360.html
  - apaprocki 13y ago
    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.
  - oneeyedpigeon 13y ago
    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?
    - deleted 13y ago
      [deleted]
    - praseodym 13y ago
      Apple introduced -Weverything because suddenly changing the scope of gcc's -Wall (which indeed does not include everything) would break a lot of builds.
      - pjene 13y ago
        "suddenly" is a funny way to say "over a span of years"....
      - bentcorner 13y ago
        Is "-Weverything" inclusive of new warnings? Because they're just kicking the can if not. What's next, "-Wreallyeverything"?
        e28eta 13y ago
        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.
    - acdha 13y ago
      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.
mitchty 13y ago
PVS-Studio might be a bit unlikely however in this particular case.
pjmlp 13y ago
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.
- laureny 13y ago
  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.
  - pjmlp 13y ago
    > 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. +1
  - erichurkman 13y ago
    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.
- eliteraspberrie 13y ago
  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.
  - pjmlp 13y ago
    Fully agree, I met quite a few of those egos already.
  - 13y ago
joe_bleau 13y ago
Yep, PC-lint from Gimpel catches it: Error -> Warning 527 Unreachable code at token 'ret' (col 12)
- emmelaich 13y ago
  Nice. Does PC-lint give a lot of false negatives and positives as well?
  - joe_bleau 13y ago
    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.
_pmf_ 13y ago
> 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/ https://blog.isotoma.com/2008/05/debians-openssl-disaster/.
- cwzwarich 13y ago
  Valgrind is dynamic analysis, not static analysis.
teacup50 13y ago
5 lines of a unit test also verify this: https://github.com/landonf/Testability-CVE-2014-1266/blob/master/TestableSecurityTests/TestableSecurityTests.m#L34 https://github.com/landonf/Testability-CVE-2014-1266/blob/ma...

Aissen 13y ago

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.

throwaway_yy2Di 13y ago
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 http://gcc.gnu.org/ml/gcc-help/2011-05/msg00360.html
- apaprocki 13y ago
  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.
- oneeyedpigeon 13y ago
  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?
  - deleted 13y ago
    [deleted]
  - praseodym 13y ago
    Apple introduced -Weverything because suddenly changing the scope of gcc's -Wall (which indeed does not include everything) would break a lot of builds.
    - pjene 13y ago
      "suddenly" is a funny way to say "over a span of years"....
    - bentcorner 13y ago
      Is "-Weverything" inclusive of new warnings? Because they're just kicking the can if not. What's next, "-Wreallyeverything"?
      - e28eta 13y ago
        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.
  - acdha 13y ago
    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.

mitchty 13y ago

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

pjmlp 13y ago

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.

laureny 13y ago
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.
- pjmlp 13y ago
  > 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. +1
- erichurkman 13y ago
  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.
eliteraspberrie 13y ago
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.
- pjmlp 13y ago
  Fully agree, I met quite a few of those egos already.
- 13y ago

joe_bleau 13y ago

Yep, PC-lint from Gimpel catches it: Error -> Warning 527 Unreachable code at token 'ret' (col 12)

emmelaich 13y ago
Nice. Does PC-lint give a lot of false negatives and positives as well?
- joe_bleau 13y ago
  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.

_pmf_ 13y ago

> 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/ https://blog.isotoma.com/2008/05/debians-openssl-disaster/.

cwzwarich 13y ago
Valgrind is dynamic analysis, not static analysis.

teacup50 13y ago

5 lines of a unit test also verify this: https://github.com/landonf/Testability-CVE-2014-1266/blob/master/TestableSecurityTests/TestableSecurityTests.m#L34 https://github.com/landonf/Testability-CVE-2014-1266/blob/ma...