Booleans

I've come across a subtle bug that could be in many places in the sources. Basically, any comparrisons of booleans might be unsafe.

I first ran across it in sp_document_set_undo_sensitive in document-undo.c. In it, the param 'sensitive' is of type gboolean. At one point it is compared with doc->priv->sensitive. Ok, things seem fine so far... however...

The specific types in question come to contribute to the problem. Since the code is C and not C++, it uses gboolean instead of bool (of course, since bool doesn't really exist in plain C). gboolean, in turn, lives in glib's includes (a good place to get things from) and is typedef'd as gint.

That's where the problems come in. Since it's an int, not a true bool, any non-zero value is taken as true, and any zero value is taken as false. So far that seems OK, but let's take a quick look at some possible values:

0 == false
1 == true
-1 == true
$ffffffff == true
~0 == true
4 == true

Now, given all that, this next line of code clearly has some issues:

if (sensitive == doc->priv->sensitive) return;

Let's say that 'sensitive' is set to '~0' (all bits set. a nice safe non-false value and often used for TRUE), and that doc->priv->sensitive is set to 1. Now both integer values represent 'true', however they are not actually equal to each other, and that check will fail even when both variables are not false.

-(

Therefore, anywhere two boolean-ish variables are compared for equality or inequality they will not always evaluate properly. Additionally, since these are just int values of one type or another, they could be all over the codebase yet not noticed. (There's not something simple and definitive that I can grep all of the code for, otherwise I would have tried to find all of them myself).

So the solution would seem to be to have all eyes watching for potential problem cases and then fixing them as they are encountered.

However, there is some good news. Doing a check of a single gboolean variable is safe, since all non-zero values will be treated as true, code will work as expected. It's only the case where two values are compared that can have problems.

if ( sensitive ) // works
if ( !sensitive ) // works
if ( sensitive & foo ) // works
if ( sensitive == TRUE ) // broken
if ( sensitive != TRUE ) // broken
if ( sensitive == otherSens ) // broken
if ( sensitive != otherSens ) // broken

And even better news. There's a simple trick that's been pointed out to me that can be used to make code safe. Since a logical inversion of the value works, and will end up with a common value, just invert the variables before checking.

if ( (!sensitive) == (!TRUE) ) // works
if ( (!sensitive) != (!TRUE) ) // works
if ( (!sensitive) == (!otherSens) ) // works
if ( (!sensitive) != (!otherSens) ) // works

(Oh, and since often it appears that there are some signed-unsigned mismatches, and often values are compared to bitfields, accidental mis-comparrisons have a higher probability of existing in the current codebase.)

-- Jon Cruz 11/16/03

Since you are now using C++, the obvious way to fix your problem is to replace all int that are used as strict boolean values by type bool. The comparison between two bool will always yield the expected result.

Another way to run the intermediate state is to explictely cast to bool:

 if ((bool) sensitive == (bool) some_other_stuff) {

will always compare the way you want it.

The origin of your problem is however a consistency problem. You state that you may have different convention for representing the values false for an int. If you use only one convention, you will never run into such problem.

The simplest way to use only one convention is (in C) to declare: typedef int bool;

1. define FALSE (0)
2. define TRUE (! FALSE)

You state than one possible value for false is ~0. However, this is a poor choice as ~ is a bit operator. !0 is a better choice because ! is the boolean operator.

Hope this can help.

-- Philippe Fremy, 2003-12-19