[BRLTTY] nasty bug

[Nicolas Pitre]

OK...  I wanted to test the new speech bindings. But...

BRLTTY keyboard mappings don't work...

... on my Fedora desktop.  On my Ubuntu laptop they work.  Go figure.

So I started looking at the differences between the two systems.  
According to the source code, keyboard support depends on the presence 
of <linux/input.h>.  Yes, it is there on both systems.  Yes, this is 
reflected in config.h.  So far so good.

Hmmm... ah, the keyboard support is initialized only if 
getUinputDevice() does not return -1.  In theory it shouldn't.

Let's add traces into the code... Well, getUinputDevice() is just not 
called at all.  Incidentally, neither is startKeyboardMonitor(). Hmmm...

startKeyboardMonitor() is called from tryKeyboardMonitor() only.  And 
the later is referenced via asyncRelativeAlarm().  Well... OK.

At this point, I got to learn more about this async code.  It all looks 
sensible to me, at least at first sight.  Well, this is a lie as I had 
to look it over more than once before it made some sense, but leaving 
the IO part aside for a while, the rest finally looked fine..

Let's add more traces to discover that the alarm is properly enqueued 
and so on.  *BUT* the desired event is never dequeued......

OK, let's add more traces.  the code is becoming totally messed up, but 
I'm gonna find this bug for sure!  Grrrrr.

OK! well, the reason why the keyboard monitor event is never dequeued is 
because it is scheduled to run in 4294967043 ms, or in 49 days !!!

Time to go and dig in another part of the code.

In Programs/timing.c we may find an assortment of time manipulation 
functions.  The one that is of interest is millisecondsBetween().  It 
looks trivial enough for sure.  But adding more traces to display all 
the input arguments and the returned result, I now can perform the same 
math manually.  And results don't match.

The reason?  Let's do a quick quiz:

Given the following code, what should be the printed result?

#include <stdio.h>
int main()
{
        int x = -10;
        long y = x * 1000u;
        printf("%ld\n", y);
        return 0;
}

Answer: it depends.

On a 32-bit system, the result is -10000.

On a 64-bit system, it is 4294957296.

And of course my laptop is 32-bit while my desktop is 64-bit.

It seems that, by some strange definition of the C language, type 
promotion will always become unsigned if one of the arguments in an 
operation is unsigned (denoted by the u qualifier to the literal value 
1000).  So signed int * unsigned int = unsigned int, and then it is 
extended to a long without sign extension. .  If a long is 64 bit wide, 
what used to be the sign bit in a small negative int value makes for a 
rather large positive long value instead.

If the value 1000 is qualified as a long using ul (as in 1000ul) then we 
get the same result on 32 and 64 bit systems.  In this case the int has 
to be promoted to a wider type before performing the operation. The 
signedness of the value is preserved when the signed int is promoted to 
a signed long, and then it doesn't matter if the signed long * unsigned 
long produces an unsigned long, since it will be interpreted as a signed 
long later on due to the type of the variable the result is stored in.

So... should we fix this bug by redefining MSECS_PER_SEC and friends 
with a ul qualifier?  It turns out that this is not a proper fix either.  
Those constants are also used in divide and modulus operations in 
adjustTimeValue() which might be provided with negative milliseconds 
arguments.  And then the promotion to an unsigned operation is going to 
cause unexpected results, irrespective of the width of the arguments.

So the best fix is really to not use any qualifyers at all and let 
MSECS_PER_SEC and friends be defined as signed ints by default.

Here's the patch:

diff --git a/Programs/timing.h b/Programs/timing.h
index 112e711..2c21a2f 100644
--- a/Programs/timing.h
+++ b/Programs/timing.h
@@ -23,9 +23,9 @@
 extern "C" {
 #endif /* __cplusplus */
 
-#define MSECS_PER_SEC  1000u
-#define USECS_PER_MSEC 1000u
-#define NSECS_PER_USEC 1000u
+#define MSECS_PER_SEC  1000
+#define USECS_PER_MSEC 1000
+#define NSECS_PER_USEC 1000
 #define USECS_PER_SEC  (USECS_PER_MSEC * MSECS_PER_SEC)
 #define NSECS_PER_MSEC (NSECS_PER_USEC * USECS_PER_MSEC)
 #define NSECS_PER_SEC  (NSECS_PER_USEC * USECS_PER_MSEC * MSECS_PER_SEC)

I won't tell how many hours I spent banging my head on this one.


Nicolas