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

@@ -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];
     }

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 {