Taking a Page from the Daryl Zero Handbook

Do things right side up.

Get It Try It

As everyone knows today, OTR is a great thing, and kudos to those who helped develop it. Secure communications wouldn't be possible without it. And the code is assuredly solid.

Yet a quick look at some of the code serves as a reminder that there are some basics of logic and programming that are often overlooked. There are times when 'upside down' ('head up the arse') coding can win, but those aren't - and should never be - the rule.

Sanity checks: they're a cornerstone of programming. They're used everywhere. Sanity checks make sure that input data is correct and not out of bounds. They help prevent vulnerabilities and consequent hacks.

The great detective Daryl Zero might teach us all a thing or two about programming in this context. For it's far easier - and much more straightforward - to define what set of values work than to try to enumerate every possible value that doesn't.

Twitter UI coders make this painfully apparent. The Twitter code for shortening URLs is still going through growing pains, even as other similar services online experience no such difficulties. The Twitter code seems to have begun by using only the English alphabet, the decimal digits, and a few punctuation marks as legit characters. As for schemes, only HTTP was recognised - likely because no one gave the matter much thought. (Twitter code seems to be very 'ad hoc' at the front end, unlike the robust works going on behind the scenes.)

Then gradually that code ran into snags. Slowly it's growing on the crew there that upwards of 65,000 characters are actually allowed in URLs. Which of course presents a significant problem, as the method used began by disallowing rather than allowing.

Here's a small snipped from OTR code from this year. It's a minimal Objective-C method for initialisation.

@implementation OTRTLV

- (instancetype) initWithType:(OTRTLVType)type data:(NSData *)data {
    if (self = [super init]) {
        self.type = type;
        self.data = data;
        if (![self isValidLength]) {
            return nil;
        }
    }
    return self;
}

- (BOOL) isValidLength {
    if (!self.data || self.data.length > UINT16_MAX) {
        return NO;
    }
    return YES;
}

@end

Pay particular attention to the second method. That method wants to allow validation in almost all cases except when a method argument isn't non-zero or in range. But do we really know that no other input values will cause things to crash?

The logic used is 'upside down'. Or 'head up the arse'. The straightforward version would be as follows.

- (BOOL) isValidLength {
    return (self.data && self.data.length <= UINT16_MAX);
}

Not only is this rewrite more efficient and less long-winded, but it uses straightforward logic - a logic that also protects against future errors. The implication is obvious: only allow if data exists and is in range. Should other sanity checks be needed in the future, they need only be documented and added (mostly the same thing).

Reading the code becomes clear: a valid length is when data is not nil and when its length is less than or equal to UINT16_MAX.

The matter might seem trivial with such a simple example, but it's not.

One could also look further to the preceding method which seems to be the only caller. Why the code needs two separate methods in such case is a mystery.

Finally, the interface declaration gives pause for thought.

/**
 * returns NO if data.length > UINT16_MAX
 */
- (BOOL) isValidLength;

The purpose of the method is not only to check that the length is less than UINT16_MAX; it must also be sure that the variable data is not nil. Even better: the documentation should say 'returns YES if...'

But the OTR code is solid, and we're all grateful that it's out there.