From ec8a1b6549a059cac1d284ec8b6377f63af75d05 Mon Sep 17 00:00:00 2001 From: Nick Brassel Date: Thu, 12 Nov 2026 12:28:13 +1100 Subject: [PATCH 1/2] Fix ChibiOS control OUT buffer sizing. (#26570) * Fix ChibiOS control OUT buffer sizing. Some ChibiOS low level drivers (STM32 USBv1, WB32, Kinetis) write a whole control packet into the EP0 OUT buffer rather than only the requested length, so the buffer must hold a full packet. SET_REPORT and SET_LINE_CODING now share one packet-sized buffer, and line coding is copied out on transfer completion so a short data stage cannot leave it partly overwritten. * Use exact size. * Recover some RAM on failing builds. --- keyboards/annepro2/info.json | 2 -- keyboards/esca/getawayvan_f042/keyboard.json | 3 +-- tmk_core/protocol/chibios/usb_main.c | 19 +++++++++++++------ 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/keyboards/annepro2/info.json b/keyboards/annepro2/info.json index 8c7041005fb..7aaeb0659d2 100644 --- a/keyboards/annepro2/info.json +++ b/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, diff --git a/keyboards/esca/getawayvan_f042/keyboard.json b/keyboards/esca/getawayvan_f042/keyboard.json index 97fe0022dce..b3b96434e1c 100644 --- a/keyboards/esca/getawayvan_f042/keyboard.json +++ b/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", diff --git a/tmk_core/protocol/chibios/usb_main.c b/tmk_core/protocol/chibios/usb_main.c index 5939e3273b9..d450ec4c8b4 100644 --- a/tmk_core/protocol/chibios/usb_main.c +++ b/tmk_core/protocol/chibios/usb_main.c @@ -231,18 +231,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]); } } @@ -277,7 +277,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; @@ -549,6 +549,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) */ @@ -557,7 +564,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.*/ From 8380b05c39959f2b254aeb130c9433a732af47d5 Mon Sep 17 00:00:00 2001 From: Nick Brassel Date: Thu, 12 Nov 2026 12:28:46 +1100 Subject: [PATCH 2/2] Add sanitizers to unit tests. (#26571) * Fix out-of-bounds read in encoder test initialisation * Reject out-of-range tracks in sequencer track activation * Compare unaligned flash word bytewise in emulated EEPROM test * Abort on out-of-range access in the test platform EEPROM emulation * Add SANITIZE=yes to the native unit test build --- .github/workflows/unit_test.yml | 7 ++++++- builddefs/build_test.mk | 9 +++++++++ docs/unit_testing.md | 8 ++++++++ drivers/encoder/encoder_quadrature.c | 3 ++- platforms/test/eeprom.c | 18 ++++++++++++++---- .../eeprom_legacy_emulated_flash_tests.cpp | 3 ++- quantum/sequencer/sequencer.c | 9 +++++++++ 7 files changed, 50 insertions(+), 7 deletions(-) diff --git a/.github/workflows/unit_test.yml b/.github/workflows/unit_test.yml index a06d38d8259..37f48023334 100644 --- a/.github/workflows/unit_test.yml +++ b/.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 }} diff --git a/builddefs/build_test.mk b/builddefs/build_test.mk index a9c9e5be2ee..8ed5092fd53 100644 --- a/builddefs/build_test.mk +++ b/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 \ diff --git a/docs/unit_testing.md b/docs/unit_testing.md index aec4ec8334d..49643c9d226 100644 --- a/docs/unit_testing.md +++ b/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. diff --git a/drivers/encoder/encoder_quadrature.c b/drivers/encoder/encoder_quadrature.c index b3aeb136b2f..53d44b8741a 100644 --- a/drivers/encoder/encoder_quadrature.c +++ b/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]; } diff --git a/platforms/test/eeprom.c b/platforms/test/eeprom.c index 193a3bb4cee..c060896fc83 100644 --- a/platforms/test/eeprom.c +++ b/platforms/test/eeprom.c @@ -14,18 +14,28 @@ * along with this program. If not, see . */ +#include +#include #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) { diff --git a/platforms/test/eeprom_legacy_emulated_flash_tests.cpp b/platforms/test/eeprom_legacy_emulated_flash_tests.cpp index 41b27b621df..b5b7a9abb58 100644 --- a/platforms/test/eeprom_legacy_emulated_flash_tests.cpp +++ b/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); diff --git a/quantum/sequencer/sequencer.c b/quantum/sequencer/sequencer.c index ff243c870bb..9956f860e8b 100644 --- a/quantum/sequencer/sequencer.c +++ b/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 {