Lately, we seem to be a bit over-exuberant in our desire to point fingers while running about in circles yelling “Look! Look! [insert major firm’s name here] screwed the pooch big time!”
While I believe it important to identify and correct security issues in as expedient fashion as possible, the endless echo chamber adds nothing of value.
I’ve read the code at the center of Apple’s recent SSL security issue. Yes, it’s bad. What it isn’t is unexpected.
I’m sure the old “goto’s are the source of all kinds of badness, from security holes to acne” crowd are probably have their pitchforks and torches at the ready. I, however, will not be joining them for this outing.
Hopefully, the thoughtful reader has already:
- reviewed the code in question
- is aware of the difference between -Wall and -Weverything
- has read Lyon’s commentary
- uses static analysis tools as an adjunct to [not a replacement for] code reviews
- realizes why it is so important to fail first
- knows that this type of thing isn’t going to change any time soon
You may be asking yourself, “What is this failing first of which I speak?”
Simply put, every function/method/routine should fail first and fail fast. Far too many people are out there writing “happy path” code. This is to say their code properly handles situations where everything goes to plan. Hence, the happy bit.
This kind of code isn’t really interesting code in my opinion. Ever since my classmates in college started showing me their programs, I’ve taken great pleasure in poking holes in them. My master’s work in computer viruses was an exercise in attempting to create a design that was intended to fend off attacks. Working at GE Space and later on Visual SourceSafe gave me an appreciation of systems that did not forgive failure. As a result, when I look at a problem, I think first about what will go wrong. Not what could, but what will. As was once said, “constants aren’t and variables won’t.” Or as Fox Mulder was wont to say, “trust no one.” Parameters will be invalid, globals will change while you’re trying to use them and other routines you call won’t do what you need them to do. This includes system routines. In the time since I began using *nix systems around 1980, I’ve seen attempts to set the time crash the system, attempts to set print output size return snarky commentary, and countless weird responses to out of memory conditions. To make matters worse, many people are still fixating on making their development environment behave as though we still lived in a world where the VT100 was the newest tool in the box. We don’t want to refactor the code because the re-validation effort would be outrageous. We write our code to the least common denominator.
So, what’s a developer to do? Here is my hit list. It’s not exhaustive. It won’t end world hunger. It is entirely my opinion.
Validate All Parameters
Not some, not just pointers, not just the easy ones, but all of them.
Fail as Soon as You Fail
Don’t create a variable and continuously retest it as you go down the routine, get to the bottom, unwind and return.
Fail First
Since things can go wrong, test for them first. Failure code tends to be small, success code large. When you put the failure case after the success, it greatly reduces its connectedness to the related if statement. When you put it up front, you can immediately see what’s supposed to happen when things go south. Additionally, since we fail first, this can be done before you actually put in ‘operational’ code. This allows you to put together a framework for the code faster.
Return Something Meaningful
There are very few instances where you should create a routine that doesn’t return either a boolean or an enumeration of state. Use an error context when necessary. Don’t use a global. Ever. Ever ever. errno has got to be one of the worst ideas anyone ever had. Although errno shifted up 8 bits is right up there.
Initialize All Local Variables
The compiler may indeed initialize them for you. If you’ve got a smart compiler (they really are good these days), then this won’t cost you anything. Static analyzers will tell you to do this.
Have a Single Exit Point
Please don’t whine about how it makes your code look. You’re probably going to have other issues if you don’t. Things like memory and lock management in languages like C and C++. The future generations who are trying to debug around your routine will thank you for not making them dig through the chicken entrails to figure out how they got out of the routine.
Scope Your Variables
Having all your locals at the top of the routine because they’re easier to find says something. It’s not a good something. Did I mention that compilers were really good? Having variables in the smallest possible scope help them. Create a scope if you need to, there is no penalty from the code police for using braces
Scope Your Clauses
This includes if‘s, case‘s, for‘s and anything else that can have more than one statement. This would have made the Apple bug much more obvious.
Use the Appropriate Flow Control Structures
We can see that had Apple used else if’s instead of a series of separate if’s the code would have shown the dependent sequencing and made the goto’s unnecessary. There is a time and place for goto’s, but when they are used to such an extent as in the Apple case, it indicates structural issues. There are multiple control mechanisms for a reason. Using elaborate variable manipulation when you could use a break boggles the mind.
Stop Pretending You’re Smarter than the Compiler
If you’re not keeping up with what Intel, the gcc or the clang folks are doing with compilers or Intel is doing with profiling feedback; then don’t even try to pretend that you can provide more help to the compile than it can figure out on its own.
Use a Static Analyzer
Static Analyzers have way more patience for looking at code lifetimes than a person ever will. They don’t have attitude and will readily call your baby ugly. We need that. We have a whole boatload of ugly babies.
Make Code Reviews a Required Part of the Development Process
This may be one of the hardest things to do. It’s that ugly baby thing. It’s “the code” and not “our code.” I have yet to meet a developer who would disagree that they desire to produce the best product they can. Code reviews should be about objective measures and not bike sheds.
Test it Until it Falls Over
Good developers and good testers are like orchids. Unless you treat them well, they aren’t around very long. A good tester is a good developers best friend. A bad developer will drive off a good tester. A bad tester can kill a project. A good tester will understand the product and it’s use. They also are really sadistic people who take great pleasure in tormenting the product. What they do is find the sharp bits that are protruding and point them out. The thing about developing software is that after a while you stop thinking about what it might do and only think about what it does do. I can’t count how many times I’ve heard a developer ask, “Why would you do that?”
Be Realistic
If you don’t allow for the review feedback loop, you will suffer. It takes time to make corrections. Done is only done when you’ve exercised your error paths. If you don’t do failure testing, you will suffer. If you believe that once the happy path passes that you are ready to release, you will suffer. Notice the pattern?
Charles Wilson's Blog 