Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UBSAN complaint in TapDance (running simulation-tests) #1347

Open
hardenedapple opened this issue Jun 19, 2023 · 1 comment
Open

UBSAN complaint in TapDance (running simulation-tests) #1347

hardenedapple opened this issue Jun 19, 2023 · 1 comment
Labels
bug Something isn't working plugin Issues related to otherwise unlisted plugins

Comments

@hardenedapple
Copy link
Contributor

Describe the bug
While running simulator tests with UBSAN I get a warning with the stack trace at the end of this post.
This happens in the simulator tests issues/{980,571,806,922} and TapDance/basic.

On running in the debugger It looks like the cause is in getting an address from an empty event_queue_.
This data does end up going into user callback tapDanceAction, which seems to get bad information.

I "got rid of the warning" with the below, but don't know if that makes semantic sense.

diff --git
a/plugins/Kaleidoscope-TapDance/src/kaleidoscope/plugin/TapDance.cpp
b/plugins/Kaleidoscope-TapDance/src/kaleidoscope/plugin/TapDance.cpp
index 999485b0..3fff2eba 100644
--- a/plugins/Kaleidoscope-TapDance/src/kaleidoscope/plugin/TapDance.cpp
+++ b/plugins/Kaleidoscope-TapDance/src/kaleidoscope/plugin/TapDance.cpp
@@ -107,7 +107,7 @@ EventHandlerResult
TapDance::onKeyswitchEvent(KeyEvent &event) {
  if (event_queue_.isEmpty() && !isTapDanceKey(event.key))
    return EventHandlerResult::OK;

-  KeyAddr td_addr = event_queue_.addr(0);
+  KeyAddr td_addr = event_queue_.isEmpty() ? event.addr :
event_queue_.addr(0);
  Key td_key      = Layer.lookupOnActiveLayer(td_addr);
  uint8_t td_id   = td_key.getRaw() - ranges::TD_FIRST;
     #0 0x55978b3ad2c5 in kaleidoscope::Layer_::lookupOnActiveLayer(kaleidoscope::MatrixAddr<(unsigned char)4, (unsigned char)16>) ... src/kaleidoscope/layers.h:103
     #1 0x55978b3c49dd in kaleidoscope::plugin::TapDance::onKeyswitchEvent(kaleidoscope::KeyEvent&) ...plugins/Kaleidoscope-TapDance/src/kaleidoscope/plugin/TapDance.cpp:111
     #2 0x55978b3ab9f9 in kaleidoscope_internal::EventHandler_onKeyswitchEvent_v1_caller<true, kaleidoscope::plugin::TapDance, kaleidoscope::KeyEvent&>::call(kaleidoscope::plugin::TapDance&, kaleidoscope::KeyEvent&) ...tests/issues/423/423.ino:69
     #3 0x55978b3ab31b in kaleidoscope::EventHandlerResult kaleidoscope_internal::EventHandler_onKeyswitchEvent_v1::call<kaleidoscope::plugin::TapDance, kaleidoscope::KeyEvent&>(kaleidoscope::plugin::TapDance&, kaleidoscope::KeyEvent&) ...tests/issues/423/423.ino:69
     #4 0x55978b3aa954 in kaleidoscope::EventHandlerResult kaleidoscope_internal::EventDispatcher::apply<kaleidoscope_internal::EventHandler_onKeyswitchEvent_v1, kaleidoscope::KeyEvent&>(kaleidoscope::KeyEvent&) ...tests/issues/423/423.ino:69

Environment (please complete the following information):

  • OS: Linux
  • Version master-branch
  • Device Virtual
@hardenedapple hardenedapple added the bug Something isn't working label Jun 19, 2023
@gedankenexperimenter
Copy link
Collaborator

At first glance, I thought using event.addr was probably the wrong choice if the queue was empty, but on closer inspection, I recognized that this initial reaction was incorrect, because the event in question is a TapDance key that toggled on, and the call to tapDanceAction() on line 132 could get the incorrect address and trigger a bug. The suggested fix looks correct to me.

@gedankenexperimenter gedankenexperimenter added the plugin Issues related to otherwise unlisted plugins label Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working plugin Issues related to otherwise unlisted plugins
Projects
None yet
Development

No branches or pull requests

2 participants