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. 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 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. Here’s the code surrounding the first issue I caught. The implicit cast is highlighted in yellow. See the problem? Right, it appears that Is the behavior of this snippet supposed to flip from “keep only preceding characters” to “do nothing” when The maintainer fixed this issue by changing the bound check to The second issue I caught is inside this code: 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: 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. The last issue I caught is the most double-take worthy thing I’ve seen in months, but was ultimately harmless: 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, (The explicit cast to 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. — — —
Turn On Your Damn Warnings
Phone Numbers and Libraries
int iteration variable despite NSArray expecting an NSUInteger index, are totally harmless. Others are not so harmless.#1: Out of Bounds
// 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)];
}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.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.>0.#2: Unsigned Enum
// (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;
}unsigned x = -1LL;
printf(x == -1LL ? "true" : "false"); // prints false#3: Wat
UInt32 potentialCountryCode = -1;
int numberLength = fullNumber.length;
for (...) {
potentialCountryCode = (UInt32)[[fullNumber substringWithRange:NSMakeRange(0, i)] intValue];
...
}
return 0;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.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
Discuss on Reddit
My Twitter: @CraigGidney
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
