Fix possible repeat key infinite recursion

[windexlight]

It was possible when performing the following sequence on a fresh boot for the keyboard to hang due to infinite recursion:

1. Press repeat key
2. Press and release another key
3. Release repeat key

Only occurred in fresh boot state (i.e., repeat key needed to be the first key pressed). Have implemented a fix and added a unit test which fails before the implemented fix, and passes afterwards.

Description

The possibility of infinite recursion was mentioned in a comment at the beginning of repeat_key_invoke ("Since this function calls process_record(), it may recursively call itself"). This was apparently meant to be guarded against by checking if processing_repeat_count is non-zero, and returning immediately if it is. Later on in the function, there are these lines:

processing_repeat_count = registered_repeat_count;
process_record(&registered_record);
processing_repeat_count = 0;

So, the intention is that guards are added around the process_record() call by setting processing_repeat_count to something non-zero so that if repeat_key_invoke is called again, it would return without further recursion.

However, registered_repeat_count is set (to something non-zero) only in the block above the process_record() call if event->pressed is non-zero.

Note also that there is a guard at the beginning of the function that returns immediately if last_record.keycode is zero. Effectively, this means if set_last_record() has not been called to set the last keycode, repeat_key_invoke() would return immediately.

Now, imagine this sequence:

1. Fresh boot state.
2. Repeat key is pressed and held. Here, repeat_key_invoke() would return immediately due to the check against last_record.keycode - we don't yet have a last keycode.
3. Another key (example: 'a') is pressed and released. This results in set_last_record() being called, which then sets last_record.keycode, so that check will pass in the next call to repeat_key_invoke().
4. Repeat key is released. repeat_key_invoke() would be called again, and pass the initial guard because we have a last keycode now. Because it is a release event, the block that sets registered_repeat_count to something non-zero is never executed. So, the line before the process_record() call never sets processing_repeat_count to something non-zero, and there is no guard to prevent infinite recursion.

Proposed fix is to guard against this by wrapping the whole block containing the call to process_record() in a check to ensure that registered_repeat_count is non-zero, which ensures that the guard against infinite recursion will always be activated as intended.

Coming up with a unit test to reproduce the issue prior to the fix proved a little tricky. Because reproduction depends on static data in repeat_key.c being in a "fresh boot" state (i.e., zeroed), and because that static data is not intrinsically reset between tests, it is necessary to explicitly reset the data, or else the test will only reproduce the issue if it is the first one executed. We don't want to depend on test order, so a new function which can be used to reset the data is necessary. Some of the data needing to be reset was defined as static local within repeat_key_invoke, so in order to allow it to be externally reset (by the new test), the definitions needed to be moved out to static global scope. Further, to clearly disambiguate them from static local variables with the same names in alt_repeat_key_invoke(), the new static global variables were renamed, appending _repeat_key to the end (note this affects the final names of the variables referred to in some of the narrative above, but read that from the context of the state prior to any of the changes introduced here).

Disclaimer: I am certainly not an expert on Google Test, but could not identify any more elegant way to force a reset to a fresh boot state at the start of a test without adding the explicit reset function. I am happy to be enlightened if there is a better way.

At first glance, it may seem that alt_repeat_key_invoke() could be susceptible to the same issue, but static analysis and testing does not so far confirm this. Since it appears to be "ain't broke", choosing not to fix.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout (addition or update)
• Documentation

Issues Fixed or Closed by This PR

N/A

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).