DynamicMacros: Fortify our cache update

There were a number of problems with how we updated and handled our cache.

First of all, we did not support empty macros that consist only of a single
`MACRO_ACTION_END`: we returned immediately as soon as we encountered one such.
This is undesirable, we want to support such empty macros.

Seconds, we did _not_ bail out early when encountering an unknown step. We
continued reading from storage until we reached our slice's end. That's also
undesirable, because data past an unknown step is not something we can reliably
parse. We should have bailed out here.

On top of that, we did not keep our id->location map in good shape. The initial
cache update did the right thing, but if we did an update where we ended up with
less macros, our map would have dangling pointers for macro ids that no longer
exist. That's not a problem when our update clears the rest of the storage
slice, but if it doesn't, the results of trying to run an unknown macro would be
unpredictable. Even if we don't care about that, it's still very inefficient,
especially when we have large macro storage.

So, this update does a whole lot of things to improve the situation:

We now keep track of how many macros we find during a cache update, and will
refuse to play macro ids that are beyond our count. This improves efficiency,
and protects us from trying to run garbage.

We also support empty macros now, and return early from a cache update when we
encounter an unknown step type.

Fixes #1177.

Signed-off-by: Gergely Nagy <algernon@keyboard.io>

plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp

@@ -1,5 +1,5 @@
 /* DynamicMacros - Dynamic macro support for Kaleidoscope.
- * Copyright (C) 2019, 2021  Keyboard.io, Inc.
+ * Copyright (C) 2019-2022  Keyboard.io, Inc.
  *
  * This program is free software: you can redistribute it and/or modify it under
  * the terms of the GNU General Public License as published by the Free Software
@@ -37,6 +37,7 @@ namespace plugin {
 uint16_t DynamicMacros::storage_base_;
 uint16_t DynamicMacros::storage_size_;
 uint16_t DynamicMacros::map_[];
+uint8_t DynamicMacros::macro_count_;
 Key DynamicMacros::active_macro_keys_[];
 
 // =============================================================================
@@ -65,11 +66,10 @@ void DynamicMacros::tap(Key key) {
   Runtime.handleKeyEvent(KeyEvent(KeyAddr::none(), WAS_PRESSED | INJECTED, key));
 }
 
-void DynamicMacros::updateDynamicMacroCache() {
+uint8_t DynamicMacros::updateDynamicMacroCache() {
-  uint16_t pos              = storage_base_;
+  uint16_t pos       = storage_base_;
-  uint8_t current_id        = 0;
+  uint8_t current_id = 0;
-  macro_t macro             = MACRO_ACTION_END;
+  macro_t macro      = MACRO_ACTION_END;
-  bool previous_macro_ended = false;
 
   map_[0] = 0;
 
@@ -79,7 +79,6 @@ void DynamicMacros::updateDynamicMacroCache() {
     case MACRO_ACTION_STEP_EXPLICIT_REPORT:
     case MACRO_ACTION_STEP_IMPLICIT_REPORT:
     case MACRO_ACTION_STEP_SEND_REPORT:
-      previous_macro_ended = false;
       break;
 
     case MACRO_ACTION_STEP_INTERVAL:
@@ -87,19 +86,16 @@ void DynamicMacros::updateDynamicMacroCache() {
     case MACRO_ACTION_STEP_KEYCODEDOWN:
     case MACRO_ACTION_STEP_KEYCODEUP:
     case MACRO_ACTION_STEP_TAPCODE:
-      previous_macro_ended = false;
       pos++;
       break;
 
     case MACRO_ACTION_STEP_KEYDOWN:
     case MACRO_ACTION_STEP_KEYUP:
     case MACRO_ACTION_STEP_TAP:
-      previous_macro_ended = false;
       pos += 2;
       break;
 
     case MACRO_ACTION_STEP_TAP_SEQUENCE: {
-      previous_macro_ended = false;
       uint8_t keyCode, flags;
       do {
         flags   = Runtime.storage().read(pos++);
@@ -109,7 +105,6 @@ void DynamicMacros::updateDynamicMacroCache() {
     }
 
     case MACRO_ACTION_STEP_TAP_CODE_SEQUENCE: {
-      previous_macro_ended = false;
       uint8_t keyCode, flags;
       do {
         keyCode = Runtime.storage().read(pos++);
@@ -119,14 +114,17 @@ void DynamicMacros::updateDynamicMacroCache() {
 
     case MACRO_ACTION_END:
       map_[++current_id] = pos - storage_base_;
-
-      if (previous_macro_ended)
-        return;
-
-      previous_macro_ended = true;
       break;
+
+    default:
+      // When we encounter an unknown step type, stop processing. Whatever we
+      // encounter after is unknown, and there's no guarantee we can parse it
+      // properly.
+      return current_id;
     }
   }
+
+  return current_id;
 }
 
 // public
@@ -136,6 +134,11 @@ void DynamicMacros::play(uint8_t macro_id) {
   uint16_t pos;
   Key key;
 
+  // If the requested ID is higher than the number of macros we found during the
+  // cache update, bail out. Our map beyond `macro_count_` is unreliable.
+  if (macro_id >= macro_count_)
+    return;
+
   pos = storage_base_ + map_[macro_id];
 
   while (true) {
@@ -279,7 +282,7 @@ EventHandlerResult DynamicMacros::onFocusEvent(const char *command) {
         Runtime.storage().update(storage_base_ + pos++, b);
       }
       Runtime.storage().commit();
-      updateDynamicMacroCache();
+      macro_count_ = updateDynamicMacroCache();
     }
   }
 
@@ -296,7 +299,7 @@ EventHandlerResult DynamicMacros::onFocusEvent(const char *command) {
 void DynamicMacros::reserve_storage(uint16_t size) {
   storage_base_ = ::EEPROMSettings.requestSlice(size);
   storage_size_ = size;
-  updateDynamicMacroCache();
+  macro_count_  = updateDynamicMacroCache();
 }
 
 }  // namespace plugin

plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.h

@@ -1,5 +1,5 @@
 /* DynamicMacros - Dynamic macro support for Kaleidoscope.
- * Copyright (C) 2019, 2021  Keyboard.io, Inc.
+ * Copyright (C) 2019-2022  Keyboard.io, Inc.
  *
  * This program is free software: you can redistribute it and/or modify it under
  * the terms of the GNU General Public License as published by the Free Software
@@ -50,7 +50,8 @@ class DynamicMacros : public kaleidoscope::Plugin {
   static uint16_t storage_base_;
   static uint16_t storage_size_;
   static uint16_t map_[31];
-  static void updateDynamicMacroCache();
+  static uint8_t macro_count_;
+  static uint8_t updateDynamicMacroCache();
   static Key active_macro_keys_[MAX_CONCURRENT_DYNAMIC_MACRO_KEYS];
   static void press(Key key);
   static void release(Key key);