Turn On Your Damn Warnings

Turn On Your Damn Warnings

Does your build environment have optional warnings? Turn them on. All of them. This has been said hundreds of times before, but I ran into lack-of-warnings problems in practice last week so I get to say it again.

Originally this post was going to be an ‘unfathomable bugs’ post. I toned it down because a) not turning on warnings is pretty fathomable, b) there weren’t many mistakes given the size of the library in question, and c) the maintainer fixed the issues I reported within a day.

Phone Numbers and Libraries

On a current project, we need the ability to parse and format phone numbers. Phone numbers are a great example of a system that’s way more complicated than you think. Handling them yourself is insane, so of course I went looking for a library that would do the heavy lifting for us (and work on iOS).

I soon found libPhoneNumber-iOS, the Objective-C port of Google’s phone number handling library libPhoneNumber. This is an ideal choice for us, because our iOS app has to interop with an app on Android (meaning it uses libPhoneNumber, and we can benefit from behaving identically).

The installation instructions for libPhoneNumber-iOS are straightforward: drop source files X through Y into your project.

Well, okay.

Ugh. Hundreds of warnings. Mostly about implicit sign conversions. C has a nasty tendency to re-interpret negative values as really large positive values, so it’s important to have these warnings turned on. The dev(s)/maintainer(s) of libPhoneNumber-iOS don’t, but we do.

Most implicit sign conversions, caused by things like using an int iteration variable despite NSArray expecting an NSUInteger index, are totally harmless. Others are not so harmless.

I decided to go through the code and make the casts explicit while checking that they are indeed harmless. I caught three issues serious enough to report.

#1: Out of Bounds

Here’s the code surrounding the first issue I caught. The implicit cast is highlighted in yellow. See the problem?

// Check for extra numbers at the end.
int secondNumberStart = [self stringPositionByRegex:number regex:self.SECOND_NUMBER_START_PATTERN_];
if (secondNumberStart >= 0) {
    possibleNumber = [possibleNumber substringWithRange:NSMakeRange(0, secondNumberStart - 1)];
}

Right, it appears that secondNumberStart can be 0. Thus the expression secondNumberStart - 1 can evaluate to -1, and become 4294967295 due to being cast to the NSUInteger expected by NSMakeRange. This causes the substring operation to go from returning an empty string to returning the entire input string.

Is the behavior of this snippet supposed to flip from “keep only preceding characters” to “do nothing” when SECOND_NUMBER_START_PATTERN occurs at the start of the string instead of afterwards? Is the root cause the too-lenient bounds check (>= 0), the unexplained extra character being chopped off (- 1), the incorrect expectation of saturating arithmetic, or something else entirely? I don’t know.

The maintainer fixed this issue by changing the bound check to >0.

#2: Unsigned Enum

The second issue I caught is inside this code:

// (in header)
typedef enum {
    NBEValidationResultIS_POSSIBLE = 0,
    NBEValidationResultINVALID_COUNTRY_CODE = 1,
    NBEValidationResultTOO_SHORT = 2,
    NBEValidationResultTOO_LONG = 3
} NBEValidationResult;

// (in implementation)
-(NBEValidationResult)isPossibleNumberWithReason:(NBPhoneNumber*)number error:(NSError *__autoreleasing *)error {
    NBEValidationResult res = -1;
    @try {
        res = [self isPossibleNumberWithReason:number];
    } @catch (NSException *exception) {
        ...
    }
    return res;
}

It’s clear that the highlighted -1, where the implicit cast is occurring, was intended as a “bad result” result. However, because enums are not guaranteed to be signed in C, the method is actually returning 2^32-1. Or maybe 2^16-1. Or some other value. It’s implementation and architecture specific.

It would be so easy to accidentally compare that pseudo-negative-one against a slightly different pseudo-negative-one and have them compare not equal due to different implicit casts. For example, this code prints false when I run it on c fiddle:

unsigned x = -1LL;
printf(x == -1LL ? "true" : "false"); // prints false

Interestingly, C’s counter-intuitive semantics when doing signed-vs-unsigned comparisons tend to hide this problem. Usually you’ll be comparing the result of the function against -1, causing it to undergo the same implicit conversion as the -1 that was returned, and so they end up equal. (Important ingredient for a great language: two wrongs make a right.)

The maintainer fixed this issue by adding an UNKNOWN constant to the enum, to be used instead of -1.

#3: Wat

The last issue I caught is the most double-take worthy thing I’ve seen in months, but was ultimately harmless:

UInt32 potentialCountryCode = -1;
int numberLength = fullNumber.length;
    
for (...) {
    potentialCountryCode = (UInt32)[[fullNumber substringWithRange:NSMakeRange(0, i)] intValue];
    ...
}
    
return 0;

Yeah. That’s pretty blatant. Initializing an unsigned variable with a negative value.

Don’t worry. The -1 which is actually a 2^32-1 isn’t used. As you can see in the excerpt, potentialCountryCode is either never read or always assigned a different value before being read. The variable should actually be declared inside the for loop, and the -1 constant should disappear.

(The explicit cast to UInt32 inside the loop is far more worrying here, although it doesn’t trigger any warnings. Is fullNumber guaranteed to be free of hyphens, which would cause a negative int value? Then why use intValue instead of unsignedIntValue?)

Summary

Turn on your warnings. This is especially important if you’re writing a library that is installed by including the source, since users might try to include it in a project that does have warnings on.

Discuss on Reddit

My Twitter: @CraigGidney

Comments are closed.

Twisted Oak Studios offers consulting and development on high-tech interactive projects. Check out our portfolio, or Give us a shout if you have anything you think some really rad engineers should help you with.

Archive