Implicit Comparrisons

Posted: July 4, 2008 in Subtle bugs, Testing, Thoughts on development
Tags: , , ,

I’ve recently had a period where I didn’t do much else than writing unit tests. Instead of just writing them like a robot I tried breaking as much of the code as possible while getting our coverage percentage up.

There was one construct that kept coming back: Implicit comparisons. When I say implicit comparison what I basically mean is a comparison using a different comparison operator than ‘==’ or where at least one side of the comparison is an expression where not all possible values are meaningful.
//implicit
x < 8 //implicit
“ok” == IsUserLoggedIn()
//Not all possible values of IsUserLoggedIn are meaningful
//“I’m Santa Claus” for one is probably not meaningful.
//This also serves as an example of why not to represent state as strings

Sometimes the implicitness is hard to spot and sometimes the result of not spotting them might make the system rather vulnerable.
Let’s say that we in a system have a permissions check using an external method call GetPermissions().
Let’s assume that the possible values for PermissionFlags are None, Read, Write and Full (integer values 1,2,4,8).
GetPermissions returns a PermissionsFlags value.
The implicit comparison could then be similar to:
var neededPermission = PermissionFlags.Full;
if (neededPermission == GetPermissions(currentUser) & neededPermission) {
//Do something that requires PermissionFlags.Full permissions
}

The above code is pretty hard to test even though it’s only 2 lines, mostly because the “ugly” cases might not be easily spotted.
If GetPermissions behaves nicely it should only return even values from 2-14 or 1 but it’s external so we have no way of ensuring that it is well-behaved.

For uneven numbers the comparison might still work as long as it’s ok to ignore that the none bit is set high.
A value of PermissionsFlags.None | PermissionsFlags.Full is rather ambiguous but might be meaningful based on specifications.
What happens then if GetPermissions, when passed an unknown user returns -1 as an error code, expecting the caller to handle the undefined value?
The above comparison would then work fine for all known users but might (depending on how integer values are represented) return true for all unknown users

My point with this example is twofold. Always use explicit comparisons (especially in security code) and always return a well defined set of values or if the method is external always validate the returned values before relying on them being within certain boundaries.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s