Allegro.cc - Online Community

Allegro.cc Forums » Allegro Development » bugs in key buffering, 4.2.1 and 4.3.0

This thread is locked; no one can reply to it. rss feed Print
bugs in key buffering, 4.2.1 and 4.3.0
orz
Member #565
August 2000

These are theoretical bugs - they depend upon race conditions or unusual circumstances that are rare and difficult to reproduce. I have not managed to actually observe them in practice. So, it could all be in my head. But I don't think so : )

Bug #1: Re-entry to keyboard handling code, particularly add_key()

On all platforms, in both Allegro 4.2.1 and 4.3.0, add_key() can be called even when it is already executing. This can happen in the following ways:
add_key reentry via simulate_keypress: On both Allegro 4.2.1 and 4.3.0, the user thread can call simulate_keypress while the keyboard handler was already calling add_key() or vice versa. On some platforms, the keyboard handler runs in an interrupt context or signal handler, preventing the user code from calling simulate_keypress on single-CPU machines, but this does not prevent the reverse from happening.
add_key reentry via key repeat mechanisms: In Allegro 4.2.1 (but not 4.3.0, I think), the key repeat mechanism can cause portions of the keyboard handler including simulate_keypress to run even while the regular keyboard handler (or simulate_keypress) is running.

In both Allegro 4.2.1 and 4.3.0 add_key attempts to make itself semi-safe for reentry via this naive locking mechanism:

   buffer->lock++;

   if (buffer->lock != 1) {
      buffer->lock--;
      return;
   }
   ...
   buffer->lock--;

which doesn't work reliably, and makes the consequences of reentry potentially much worse. Even when it does work, it deliberately drops a keypress. In Allegro 4.2.1 there is an additional mechanism used:

   if ((waiting_for_input) && (keyboard_driver) && (keyboard_driver->stop_waiting_for_input))
      keyboard_driver->stop_waiting_for_input();

But this mechanism is insufficient (and doesn't seem to do anything useful).

Problems this can cause:
1. If the initialization of the variable "c" is reached in a second call to add_key() before the first call writes its value of "c" back to "buffer->end", then only one of the two keypresses would show up in the buffer. This results in one keypress being reported instead of the two that happened. Furthermore, the reported keypress could be an impossible mix of the the two that actually happened, depending upon the timing of the writes to the arrays within the buffer. In such cases, the reported keypress would have the scancode of one of the actual keypresses, but the symbol of the other keypress.
2. More seriously, the "buffer->lock" variable could be left in an invalid state after all calls to add_key() returned. This would prevent all future keypresses from being added to the buffer. In order for this to happen, both calls to add_key() would have to read "buffer->lock" for the line "buffer->lock++" before the other had written back the incremented value, leaving "buffer->lock" with a value of 1, then at the end the calls would have to have their "buffer->lock--" statements not overlap, causing the value to fall to -1.

Both of those problems can't happen when the code is called from within a signal handler or interrupt (including DOS, MacOS9, and, under most circumstances, Linux), because the relative timings of reads and writes for the problems to occur cannot happen since one call pauses the other until it completes. So the only problem on DOS or MacOS9 would be a dropped keypress, which the code is designed to produce occasionally anyway.

.
Bug #2: Updating of the key buffer is handled in a manner that is unsafe on non-x86 systems.

This one is somewhat counter-intuitive, but here's how it works:
ureadkey() checks to see if a keypress is ready in the buffer by comparing "key_buffer.start == key_buffer.end". If that condition is met, it immediately returns the next entry in the buffer. It does not perform any locking. Therefore, it could return an invalid keypress if "keybuffer.end" was updated for a keypress before the buffer entry for the keypress was initialized.
This shouldn't matter, because add_key carefully writes the buffer entry before it updates "buffer->end". The problem is, the hardware or the compiler might decide to change the order of the writes. On some compilers, including MSVC, the "volatile" keyword is enough to prevent the compiler from changing the order. I'm not 100% sure that gcc and Watcomm also do this, but lets pretend they do for the moment. That still leaves problems if the hardware decides to reorder the writes. Of course, the hardware is required to make things seem as if it didn't reorder the writes, otherwise the compiler would have to do something to fix things up. BUT... in many multi-CPU environments, writes committed by one processor may be seen out of order by other processors. This never happens on x86-based systems. But, on PowerPC it does happen. So, on OSX/PPC systems with multiple CPUs, this method is not safe. Here is some documentation on the subject:

Memory Ordering in Modern Microprocessors, Part I and Part II
http://www.linuxjournal.com/article/8211
http://www.linuxjournal.com/article/8212
In particular, note the code example in part 2, which does basically the same thing that Allegro does, but does it correctly, because it adds an smp_wmb() macro.

This briefly mentions various memory consistency models used by various CPUs:
http://www.cs.cmu.edu/~acw/15740/mcm.html

Intel discusses their memory consistency model in section 7.2 of this document:
ftp://download.intel.com/design/Pentium4/manuals/25366821.pdf

edit: Problems this can cause:
This can cause a readkey() or ureadkey() call to return the wrong keypress. The returned keypress might be the keypress from 64 keypresses ago, or it might be a mix of that and the current keypress (the scancode of one an the symbol of the other).

Fixes for these bugs:

add_key() should lock to prevent reentry into it. Better yet, the locking could be done by the callers to it - simulate_keypress() and _handle_key(), which would also prevent reentry into user callbacks. The possibility of reentry into user callbacks is not mentioned in the docs (indeed, the docs imply otherwise, stating that the callbacks occur "in an interrupt context"). Casual testing for reentry will (incorrectly) suggest that it doesn't happen, since the circumstances that trigger it are rare and difficult to reproduce.

Either something equivalent to linuxes smp_wmb() should be added to add_key() or locking should be added to ureadkey().

Elias
Member #358
May 2000

In 4.3, key presses are delivered via the events system, which always does proper locking (see src/event.c). So no problems should exist there I hope.

Still, for 4.2 (and the 4.x compatibility layer in the 4.3 code which you probably were looking at) you are probably right. At the time this code was written, there was no SMP - not even threads for that matter :)

Still, easy fix is, simply add a lock, after all what for do we have a complete locking API in 4.2.

--
"Either help out or stop whining" - Evert

orz
Member #565
August 2000

Quote:

In 4.3, key presses are delivered via the events system, which always does proper locking (see src/event.c). So no problems should exist there I hope.

Still, for 4.2 (and the 4.x compatibility layer in the 4.3 code which you probably were looking at) you are probably right.

Yeah, I was referring to the compatibility layer. The purely new stuff looks correct.

Quote:

At the time this code was written, there was no SMP - not even threads for that matter :)

Yipes!

Quote:

Still, easy fix is, simply add a lock, after all what for do we have a complete locking API in 4.2.

Yes... just realize it requires locks for both add_key and ureadkey (unless a memory barrier is added to add_key), plus extra if you want user callbacks to not be reentrant.

Go to: