Merge pull request #1107 from gedankenexperimenter/bug/event-queue-leak

Fix Qukeys memory bounds violation

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

@@ -88,9 +88,6 @@ EventHandlerResult Qukeys::afterEachCycle() {
     return EventHandlerResult::OK;
   }
 
-  // Process as many events as we can from the queue.
-  while (processQueue());
-
   // If we get here, that means that the first event in the queue is a qukey
   // press. All that's left to do is to check if it's been held long enough that
   // it has timed out.
@@ -101,6 +98,10 @@ EventHandlerResult Qukeys::afterEachCycle() {
                     queue_head_.primary_key : queue_head_.alternate_key;
     flushEvent(event_key);
   }
+
+  // Process as many events as we can from the queue.
+  while (processQueue());
+
   return EventHandlerResult::OK;
 }
 

src/kaleidoscope/KeyAddrEventQueue.h

@@ -103,6 +103,8 @@ class KeyAddrEventQueue {
   // rather than using a ring buffer because we expect it will be called much
   // less often than the queue is searched via a for loop.
   void remove(uint8_t n = 0) {
+    if (n >= length_ || length_ == 0)
+      return;
     // assert(length > n);
     --length_;
     for (uint8_t i{n}; i < length_; ++i) {

tests/issues/1107/1107.ino

@@ -0,0 +1,71 @@
+/* -*- mode: c++ -*-
+ * Copyright (C) 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
+ * Foundation, version 3.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "Kaleidoscope.h"
+
+// *INDENT-OFF*
+KEYMAPS(
+  [0] = KEYMAP_STACKED
+  (
+    XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX
+   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX
+   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX
+   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX
+   ,XXX   ,XXX   ,XXX   ,XXX
+   ,XXX
+
+   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX
+   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX
+          ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX
+   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX   ,XXX
+   ,XXX   ,XXX   ,XXX   ,XXX
+   ,XXX
+  )
+) // KEYMAPS(
+// *INDENT-ON*
+
+#include <kaleidoscope/KeyAddrEventQueue.h>
+
+namespace kaleidoscope {
+namespace plugin {
+
+class QueueLeaker : public Plugin {
+ public:
+  EventHandlerResult afterEachCycle() {
+    // Shift queue without checking to be sure that queue is not empty.  If
+    // KeyAddrEventQueue uses an unsigned integer for queue length, this will
+    // result in some bad behavior.
+    queue_.shift();
+    return EventHandlerResult::OK;
+  }
+ private:
+  KeyAddrEventQueue<8> queue_;
+};
+
+} // namespace plugin
+} // namespace kaleidoscope
+
+kaleidoscope::plugin::QueueLeaker QueueLeaker;
+
+KALEIDOSCOPE_INIT_PLUGINS(QueueLeaker);
+
+void setup() {
+  Kaleidoscope.setup();
+}
+
+void loop() {
+  Kaleidoscope.loop();
+}

tests/issues/1107/sketch.json

@@ -0,0 +1,6 @@
+{
+  "cpu": {
+    "fqbn": "keyboardio:virtual:model01",
+    "port": ""
+  }
+}

tests/issues/1107/test/testcase.cpp

@@ -0,0 +1,99 @@
+/* -*- mode: c++ -*-
+ * Copyright (C) 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
+ * Foundation, version 3.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "testing/setup-googletest.h"
+
+SETUP_GOOGLETEST();
+
+namespace kaleidoscope {
+namespace testing {
+namespace {
+
+constexpr uint32_t t_0{0};
+constexpr uint32_t t_1{1};
+constexpr uint32_t t_10{10};
+
+using ::testing::IsEmpty;
+
+class KeyAddrEventQueueLeak : public VirtualDeviceTest {
+ public:
+  void assertTimeElapses(uint32_t interval) {
+    // Record time at start
+    uint32_t start = Kaleidoscope.millisAtCycleStart();
+
+    // Run cycles until the given amount of time has elapsed
+    sim_.RunForMillis(interval);
+
+    // Record time at end
+    uint32_t end = Kaleidoscope.millisAtCycleStart();
+
+    ASSERT_EQ((end - start), interval);
+  }
+};
+
+TEST_F(KeyAddrEventQueueLeak, TimeIsConsistent) {
+  assertTimeElapses(1);
+  assertTimeElapses(1);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+  assertTimeElapses(10);
+}
+
+}  // namespace
+}  // namespace testing
+}  // namespace kaleidoscope