From 2dbf0f807bd439a959f04621c6c01e68d9ad1735 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 24 May 2022 12:29:36 +0200 Subject: [PATCH 1/4] DynamicMacros: Do not read past storage_size_ when updating our map When updating our map via Focus, do not read past `storage_size_`, because we do not want to clobber storage space past our slice by accident. Signed-off-by: Gergely Nagy --- .../src/kaleidoscope/plugin/DynamicMacros.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp index c80fd15a..eb0ec223 100644 --- a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp +++ b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp @@ -272,7 +272,7 @@ EventHandlerResult DynamicMacros::onFocusEvent(const char *command) { } else { uint16_t pos = 0; - while (!::Focus.isEOL()) { + while (!::Focus.isEOL() && pos < storage_size_) { uint8_t b; ::Focus.read(b); From c0b99d763e858f45fa3917b131ee11f20338fb0d Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 24 May 2022 12:38:22 +0200 Subject: [PATCH 2/4] 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 --- .../src/kaleidoscope/plugin/DynamicMacros.cpp | 39 ++++++++++--------- .../src/kaleidoscope/plugin/DynamicMacros.h | 5 ++- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp index eb0ec223..0cf6438b 100644 --- a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp +++ b/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() { - uint16_t pos = storage_base_; - uint8_t current_id = 0; - macro_t macro = MACRO_ACTION_END; - bool previous_macro_ended = false; +uint8_t DynamicMacros::updateDynamicMacroCache() { + uint16_t pos = storage_base_; + uint8_t current_id = 0; + macro_t macro = MACRO_ACTION_END; 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 diff --git a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.h b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.h index 1678187f..0c2b045f 100644 --- a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.h +++ b/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); From 39e607e7c75413afd903c110b1af59b3422d97d4 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 24 May 2022 16:31:37 +0200 Subject: [PATCH 3/4] DynamicMacros: Fix the size of our lookup table Our lookup table should have 32 entries, not 31, as Kaleidoscope-Ranges gives DynamicMacros 32 entries. Thanks @gedankenexperimenter for spotting this! Signed-off-by: Gergely Nagy --- .../src/kaleidoscope/plugin/DynamicMacros.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.h b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.h index 0c2b045f..90545294 100644 --- a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.h +++ b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.h @@ -49,7 +49,7 @@ class DynamicMacros : public kaleidoscope::Plugin { private: static uint16_t storage_base_; static uint16_t storage_size_; - static uint16_t map_[31]; + static uint16_t map_[32]; static uint8_t macro_count_; static uint8_t updateDynamicMacroCache(); static Key active_macro_keys_[MAX_CONCURRENT_DYNAMIC_MACRO_KEYS]; From d29ffa72dceb8c40c257fe6b37fe60122150b7ea Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 24 May 2022 16:42:56 +0200 Subject: [PATCH 4/4] DynamicMacros: Make sure we keep within bounds during play Caught by @gedankenexperimenter, thanks! Signed-off-by: Gergely Nagy --- .../src/kaleidoscope/plugin/DynamicMacros.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp index 0cf6438b..52f5b6d9 100644 --- a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp +++ b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp @@ -141,7 +141,7 @@ void DynamicMacros::play(uint8_t macro_id) { pos = storage_base_ + map_[macro_id]; - while (true) { + while (pos < storage_base_ + storage_size_) { switch (macro = Runtime.storage().read(pos++)) { case MACRO_ACTION_STEP_EXPLICIT_REPORT: case MACRO_ACTION_STEP_IMPLICIT_REPORT: