Merge remote-tracking branch 'origin/develop' into xap

.github/workflows/unit_test.yml

@@ -24,6 +24,11 @@ jobs:
   test:
     runs-on: ubuntu-latest
 
+    strategy:
+      fail-fast: false
+      matrix:
+        sanitize: ['no', 'yes']
+
     container: ghcr.io/qmk/qmk_cli
 
     steps:
@@ -35,4 +40,4 @@ jobs:
       run: pip3 install -r requirements-dev.txt
 
     - name: Run tests
-      run: qmk test-c -j $(nproc)
+      run: qmk test-c -j $(nproc) -e SANITIZE=${{ matrix.sanitize }}

builddefs/build_test.mk

@@ -36,6 +36,15 @@ $(GTEST_OUTPUT)_INC := $(GTEST_INC) $(GTEST_INTERNAL_INC)
 LDFLAGS += -lstdc++ -lpthread -shared-libgcc
 CREATE_MAP := no
 
+# Build and run the tests under AddressSanitizer and UndefinedBehaviorSanitizer with `make test:all SANITIZE=yes`.
+SANITIZE ?= no
+ifeq ($(strip $(SANITIZE)),yes)
+SANITIZE_FLAGS := -fsanitize=address,undefined -fno-sanitize-recover=all -fno-omit-frame-pointer
+CFLAGS += $(SANITIZE_FLAGS)
+CXXFLAGS += $(SANITIZE_FLAGS)
+LDFLAGS += $(SANITIZE_FLAGS)
+endif
+
 VPATH += \
 	$(LIB_PATH)/googletest \
 	$(LIB_PATH)/googlemock \

docs/unit_testing.md

@@ -52,6 +52,14 @@ make test:all DEBUG=1
 
 Alternatively, add `CONSOLE_ENABLE=yes` to the tests `rules.mk`.
 
+The tests can also be built and run under AddressSanitizer and UndefinedBehaviorSanitizer, which abort on the first out-of-bounds access, use-after-free, or undefined operation:
+
+```
+make test:all SANITIZE=yes
+```
+
+The emulated EEPROM used by the tests aborts in the same way on any access outside its configured size.
+
 ## Full Integration Tests
 
 It's not yet possible to do a full integration test, where you would compile the whole firmware and define a keymap that you are going to test. However there are plans for doing that, because writing tests that way would probably be easier, at least for people that are not used to unit testing.

drivers/encoder/encoder_quadrature.c

@@ -9,6 +9,7 @@
 #include "action.h"
 #include "keycodes.h"
 #include "wait.h"
+#include "util.h"
 
 #ifdef SPLIT_KEYBOARD
 #    include "split_util.h"
@@ -123,7 +124,7 @@ void encoder_driver_init(void) {
     memset(encoder_pulses, 0, sizeof(encoder_pulses));
     const pin_t encoders_pad_a_left[] = ENCODER_A_PINS;
     const pin_t encoders_pad_b_left[] = ENCODER_B_PINS;
-    for (uint8_t i = 0; i < thisCount; i++) {
+    for (uint8_t i = 0; i < thisCount && i < ARRAY_SIZE(encoders_pad_a_left); i++) {
         encoders_pad_a[i] = encoders_pad_a_left[i];
         encoders_pad_b[i] = encoders_pad_b_left[i];
     }

keyboards/annepro2/info.json

@@ -53,8 +53,6 @@
            "pixel_fractal": true,
            "pixel_flow": true,
            "pixel_rain": true,
-           "typing_heatmap": true,
-           "digital_rain": true,
            "solid_reactive_simple": true,
            "solid_reactive": true,
            "solid_reactive_wide": true,

keyboards/esca/getawayvan_f042/keyboard.json

@@ -38,8 +38,7 @@
             "christmas": true,
             "static_gradient": true,
             "rgb_test": true,
-            "alternating": true,
-            "twinkle": true
+            "alternating": true
         }
     },
     "processor": "STM32F042",

platforms/test/eeprom.c

@@ -14,18 +14,28 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <stdio.h>
+#include <stdlib.h>
 #include "eeprom.h"
 
 static uint8_t buffer[TOTAL_EEPROM_BYTE_COUNT];
 
-uint8_t eeprom_read_byte(const uint8_t *addr) {
+// Abort on any access outside the emulated EEPROM rather than silently reading or corrupting whatever follows the backing array.
+static uintptr_t checked_offset(const void *addr) {
     uintptr_t offset = (uintptr_t)addr;
-    return buffer[offset];
+    if (offset >= TOTAL_EEPROM_BYTE_COUNT) {
+        fprintf(stderr, "eeprom: address %#zx is outside the emulated EEPROM (%zu bytes)\n", (size_t)offset, (size_t)TOTAL_EEPROM_BYTE_COUNT);
+        abort();
+    }
+    return offset;
+}
+
+uint8_t eeprom_read_byte(const uint8_t *addr) {
+    return buffer[checked_offset(addr)];
 }
 
 void eeprom_write_byte(uint8_t *addr, uint8_t value) {
-    uintptr_t offset = (uintptr_t)addr;
-    buffer[offset]   = value;
+    buffer[checked_offset(addr)] = value;
 }
 
 uint16_t eeprom_read_word(const uint16_t *addr) {

platforms/test/eeprom_legacy_emulated_flash_tests.cpp

@@ -227,7 +227,8 @@ TEST_F(EepromStm32Test, TestWriteWord) {
     /* Check values */
     /* Direct compacted-area */
     EXPECT_EQ(*(uint16_t*)&FlashBuf[EEPROM_BASE], (uint16_t)~0xdead);
-    EXPECT_EQ(*(uint16_t*)&FlashBuf[EEPROM_BASE + 3], (uint16_t)~0xbeef);
+    EXPECT_EQ(FlashBuf[EEPROM_BASE + 3], (uint8_t)~0xef);
+    EXPECT_EQ(FlashBuf[EEPROM_BASE + 4], (uint8_t)~0xbe);
     EXPECT_EQ(*(uint16_t*)&FlashBuf[EEPROM_BASE + 200], (uint16_t)~0xabcd);
     EXPECT_EQ(FlashBuf[EEPROM_BASE + 203], (uint8_t)~0x76);
     EXPECT_EQ(FlashBuf[EEPROM_BASE + 204], (uint8_t)~0x98);

quantum/sequencer/sequencer.c

@@ -70,10 +70,16 @@ void sequencer_set_track_notes(const uint16_t track_notes[SEQUENCER_TRACKS]) {
 }
 
 bool is_sequencer_track_active(uint8_t track) {
+    if (track >= SEQUENCER_TRACKS) {
+        return false;
+    }
     return (sequencer_internal_state.active_tracks >> track) & true;
 }
 
 void sequencer_set_track_activation(uint8_t track, bool value) {
+    if (track >= SEQUENCER_TRACKS) {
+        return;
+    }
     if (value) {
         sequencer_internal_state.active_tracks |= (1 << track);
     } else {
@@ -87,6 +93,9 @@ void sequencer_toggle_track_activation(uint8_t track) {
 }
 
 void sequencer_toggle_single_active_track(uint8_t track) {
+    if (track >= SEQUENCER_TRACKS) {
+        return;
+    }
     if (is_sequencer_track_active(track)) {
         sequencer_internal_state.active_tracks = 0;
     } else {

tmk_core/protocol/chibios/usb_main.c

@@ -235,18 +235,18 @@ static void usb_event_cb(USBDriver *usbp, usbevent_t event) {
  * Other Device    Required    Optional    Optional    Optional    Optional    Optional
  */
 
-static uint8_t _Alignas(4) set_report_buf[2];
+static uint8_t _Alignas(4) control_out_buf[FIXED_CONTROL_ENDPOINT_SIZE]; // Some low level drivers copy a whole control packet regardless of the requested transfer size. Shared by all EP0 OUT handlers; ChibiOS runs one control transfer at a time.
 
 static void set_led_transfer_cb(USBDriver *usbp) {
     usb_control_request_t *setup = (usb_control_request_t *)usbp->setup;
 
     if (setup->wLength == 2) {
-        uint8_t report_id = set_report_buf[0];
+        uint8_t report_id = control_out_buf[0];
         if ((report_id == REPORT_ID_KEYBOARD) || (report_id == REPORT_ID_NKRO)) {
-            usb_device_state_set_leds(set_report_buf[1]);
+            usb_device_state_set_leds(control_out_buf[1]);
         }
     } else {
-        usb_device_state_set_leds(set_report_buf[0]);
+        usb_device_state_set_leds(control_out_buf[0]);
     }
 }
 
@@ -281,7 +281,7 @@ static bool usb_requests_hook_cb(USBDriver *usbp) {
 #if defined(SHARED_EP_ENABLE) && !defined(KEYBOARD_SHARED_EP)
                             case SHARED_INTERFACE:
 #endif
-                                usbSetupTransfer(usbp, set_report_buf, sizeof(set_report_buf), set_led_transfer_cb);
+                                usbSetupTransfer(usbp, control_out_buf, 2, set_led_transfer_cb);
                                 return true;
                         }
                         break;
@@ -612,6 +612,13 @@ bool recv_midi_packet(MIDI_EventPacket_t *const event) {
  */
 static cdc_linecoding_t linecoding = {{0x00, 0x96, 0x00, 0x00}, LC_STOP_1, LC_PARITY_NONE, 8};
 
+static void set_line_coding_transfer_cb(USBDriver *usbp) {
+    if (usbGetReceiveTransactionSizeX(usbp, 0) != sizeof(linecoding)) {
+        return;
+    }
+    memcpy(&linecoding, control_out_buf, sizeof(linecoding));
+}
+
 bool virtser_usb_request_cb(USBDriver *usbp) {
     if ((usbp->setup[0] & USB_RTYPE_TYPE_MASK) == USB_RTYPE_TYPE_CLASS) { /* bmRequestType */
         if (usbp->setup[4] == CCI_INTERFACE) {                            /* wIndex (LSB) */
@@ -620,7 +627,7 @@ bool virtser_usb_request_cb(USBDriver *usbp) {
                     usbSetupTransfer(usbp, (uint8_t *)&linecoding, sizeof(linecoding), NULL);
                     return true;
                 case CDC_SET_LINE_CODING:
-                    usbSetupTransfer(usbp, (uint8_t *)&linecoding, sizeof(linecoding), NULL);
+                    usbSetupTransfer(usbp, control_out_buf, sizeof(linecoding), set_line_coding_transfer_cb);
                     return true;
                 case CDC_SET_CONTROL_LINE_STATE:
                     /* Nothing to do, there are no control lines.*/