Clean Up Your State

There’s one overriding principle of doing imperative programming safely, which experienced programmers should all have long since learned from the school of hard knocks:

Clean up your internal state before making outgoing calls

Even within your own code, calling internal methods of the same class maintained also by you, you should follow this principle. Most methods should be a block of code accessing internal data structures, followed at the very end by whatever callouts need to be done. Usually there should only be one callout, and when there are multiple ones they ideally should have APIs which allow any order of calling them to work.

To illustrate, here is a very contrived example. Let’s say that you’re working on a banking system and it has a call to transfer all the money out of one account and into another. It can look like this:

def transfer_everything_out(target) {
target.balance += self.balance;
self.balance = 0;
}

So far so good. Now let’s say that you wanted to add callback functionality to this method, like so:

def transfer_everything_out(target, callback) {
target.balance += self.balance;
callback();
self.balance = 0;
}

Obviously this is a very contrived example. Noone actually working on code handling real money would ever do things this incompetently. But it is a very simple illustration, so bear with me.

The problem here is that one of the fundamental invariants of the system — That transfers leave the sum of all balances the same — is violated at the time the callback is called. That should raise red flags based on principle, even without knowing a specific problem it causes, but a simple example of how this could be exploited is for the callback to recursively call transfer_everything_out() again, thus increasing their own balance by way too much.

There are various asinine approaches one could take to fixing this, for example putting a lock around the whole method, but any competent reviewer would tell you that the fundamental problem is the one you should have fixed on principle even before knowing that it was a security issue: The state isn’t cleaned up before the callback is called. The correct way to fix the problem is to put the lines of code in the right order:

def transfer_everything_out(target, callback) {
target.balance += self.balance;
self.balance = 0;
callback();
}

And easy as that, the problem is fixed. This approach to order of operations should be the default in all imperative code, even code without security implications, just because it results in vastly fewer bugs.