pbio/sys/storage_data: fix strict aliasing issues

Fix strict aliasing issues between block drivers and pbsys storage.

Casting a uint8_t* to a struct pointer violates strict aliasing rules,
which can lead to hard-to-find bugs. We need to ensure that the memory
being pointed to has the same alignment, etc. as the struct it is being
cast to. The usual way to do this is to use a union of the struct and
the other type we want to use (in this case, uint8_t array).

To do this, we need to move the storage data struct definition to a
header file that can be included by both pbdrv/block_device and
pbsys/storage.

Additionally, in block_device_test, the size of the ramdisk block was
wrong and was causing the NXT to crash sometime after starting the REPL.
It also incorrectly had the .noinit section attribute, but depends on
being initialized to zero, so that is removed.

Fixes: pybricks/support#2272
Fixes: ce4253a ("pbio/sys: Move block device read to driver level.")

Author: dlech

lib/pbio/drv/block_device/block_device_ev3.c

@@ -20,13 +20,6 @@
 #include <stdint.h>
 #include <string.h>
 
-#include "../core.h"
-
-#include <pbdrv/block_device.h>
-#include <pbdrv/clock.h>
-#include <pbdrv/compiler.h>
-#include <pbdrv/gpio.h>
-
 #include <tiam1808/edma.h>
 #include <tiam1808/spi.h>
 #include <tiam1808/psc.h>
@@ -36,9 +29,15 @@
 #include <tiam1808/armv5/am1808/edma_event.h>
 #include <tiam1808/armv5/am1808/interrupt.h>
 
-#include "block_device_ev3.h"
+#include "../core.h"
 #include "../drv/gpio/gpio_ev3.h"
+#include "../sys/storage_data.h"
+#include "block_device_ev3.h"
 
+#include <pbdrv/block_device.h>
+#include <pbdrv/clock.h>
+#include <pbdrv/compiler.h>
+#include <pbdrv/gpio.h>
 #include <pbio/error.h>
 #include <pbio/int_math.h>
 #include <pbio/util.h>
@@ -831,7 +830,11 @@ static struct {
      * portion of this, up to pbdrv_block_device_get_writable_size() bytes,
      * gets saved to flash at shutdown.
      */
-    uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+    union {
+        // ensure that data is properly aligned for pbsys_storage_data_map_t
+        pbsys_storage_data_map_t data_map;
+        uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+    };
 } ramdisk __attribute__((section(".noinit"), used));
 
 uint32_t pbdrv_block_device_get_writable_size(void) {
@@ -840,8 +843,8 @@ uint32_t pbdrv_block_device_get_writable_size(void) {
 
 static pbio_error_t pbdrv_block_device_load_err = PBIO_ERROR_FAILED;
 
-pbio_error_t pbdrv_block_device_get_data(uint8_t **data) {
-    *data = ramdisk.data;
+pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) {
+    *data = &ramdisk.data_map;
 
     // Higher level code can use the ramdisk data if initialization completed
     // successfully. Otherwise it should reset to factory default data.

lib/pbio/drv/block_device/block_device_flash_stm32.c

@@ -10,9 +10,8 @@
 #include <stdint.h>
 #include <string.h>
 
-#include <contiki.h>
-
 #include "../core.h"
+#include "../sys/storage_data.h"
 
 #include <pbdrv/block_device.h>
 
@@ -41,7 +40,11 @@ static struct {
      * portion of this, up to pbdrv_block_device_get_writable_size() bytes,
      * gets saved to flash at shutdown.
      */
-    uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+    union {
+        // ensure that data is properly aligned for pbsys_storage_data_map_t
+        pbsys_storage_data_map_t data_map;
+        uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+    };
 } ramdisk __attribute__((section(".noinit"), used));
 
 const uint32_t header_size = sizeof(ramdisk.saved_size) + sizeof(ramdisk.checksum_complement);
@@ -70,8 +73,8 @@ void pbdrv_block_device_init(void) {
     memcpy(&ramdisk, _pbdrv_block_device_storage_start, size);
 }
 
-pbio_error_t pbdrv_block_device_get_data(uint8_t **data) {
-    *data = ramdisk.data;
+pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) {
+    *data = &ramdisk.data_map;
     return init_err;
 }
 

lib/pbio/drv/block_device/block_device_test.c

@@ -10,8 +10,9 @@
 #include <stdint.h>
 #include <string.h>
 
-#include <pbdrv/block_device.h>
+#include "../sys/storage_data.h"
 
+#include <pbio/os.h>
 #include <pbio/version.h>
 
 /**
@@ -59,33 +60,30 @@ static const uint8_t _program_data[] = {
 // version at the right place. FIXME: Move the git version to pybricks build
 // system, since it isn't actually the micropython git version.
 #include "genhdr/mpversion.h"
-#include <pbsys/storage.h>
+
 
 static struct {
-    uint8_t user_data[PBSYS_CONFIG_STORAGE_USER_DATA_SIZE];
-    char stored_firmware_hash[8];
-    pbsys_storage_settings_t settings;
-    uint32_t program_offset;
-    uint32_t program_size;
-    uint8_t program_data[sizeof(_program_data)];
-} ramdisk __attribute__((section(".noinit"), used)) = { 0 };
+    // ensure that data is properly aligned for pbsys_storage_data_map_t
+    pbsys_storage_data_map_t data_map;
+    uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+} ramdisk;
 
 uint32_t pbdrv_block_device_get_writable_size(void) {
     return 0;
 }
 
-pbio_error_t pbdrv_block_device_get_data(uint8_t **data) {
-    *data = (void *)&ramdisk;
+pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) {
+    *data = &ramdisk.data_map;
 
     // Higher level code can use the ramdisk data if initialization completed
     // successfully. Otherwise it should reset to factory default data.
     return PBIO_SUCCESS;
 }
 
 void pbdrv_block_device_init(void) {
-    ramdisk.program_size = sizeof(_program_data);
-    memcpy(&ramdisk.stored_firmware_hash[0], MICROPY_GIT_HASH, sizeof(ramdisk.stored_firmware_hash));
-    memcpy(&ramdisk.program_data[0], _program_data, sizeof(_program_data));
+    ramdisk.data_map.slot_info[0].size = sizeof(_program_data);
+    memcpy(ramdisk.data_map.stored_firmware_hash, MICROPY_GIT_HASH, sizeof(ramdisk.data_map.stored_firmware_hash));
+    memcpy(ramdisk.data_map.program_data, _program_data, sizeof(_program_data));
 }
 
 // Don't store any data in this implementation.

lib/pbio/drv/block_device/block_device_w25qxx_stm32.c

@@ -13,9 +13,8 @@
 
 #include STM32_HAL_H
 
-#include <contiki.h>
-
 #include "../core.h"
+#include "../sys/storage_data.h"
 #include "block_device_w25qxx_stm32.h"
 
 #include <pbdrv/block_device.h>
@@ -36,15 +35,19 @@ static struct {
      * portion of this, up to pbdrv_block_device_get_writable_size() bytes,
      * gets saved to flash at shutdown.
      */
-    uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+    union {
+        // ensure that data is properly aligned for pbsys_storage_data_map_t
+        pbsys_storage_data_map_t data_map;
+        uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+    };
 } ramdisk __attribute__((section(".noinit"), used));
 
 uint32_t pbdrv_block_device_get_writable_size(void) {
     return PBDRV_CONFIG_BLOCK_DEVICE_W25QXX_STM32_SIZE - sizeof(ramdisk.saved_size);
 }
 
-pbio_error_t pbdrv_block_device_get_data(uint8_t **data) {
-    *data = ramdisk.data;
+pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) {
+    *data = &ramdisk.data_map;
 
     // Higher level code can use the ramdisk data if initialization completed
     // successfully. Otherwise it should reset to factory default data.

lib/pbio/include/pbdrv/block_device.h

@@ -11,9 +11,10 @@
 
 #include <stdint.h>
 
+#include "../sys/storage_data.h"
+
 #include <pbdrv/config.h>
 #include <pbio/error.h>
-
 #include <pbio/os.h>
 
 #if PBDRV_CONFIG_BLOCK_DEVICE
@@ -28,7 +29,7 @@
  *                      ::PBIO_ERROR_IO (driver-specific error), though boot
  *                        does not proceed if this happens.
  */
-pbio_error_t pbdrv_block_device_get_data(uint8_t **data);
+pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data);
 
 /**
  * Writes the "RAM Disk" to storage. May erase entire disk prior to writing.
@@ -57,7 +58,7 @@ uint32_t pbdrv_block_device_get_writable_size(void);
 
 #else
 
-static inline pbio_error_t pbdrv_block_device_get_data(uint8_t **data) {
+static inline pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) {
     return PBIO_ERROR_NOT_SUPPORTED;
 }
 

lib/pbio/platform/nxt/pbdrvconfig.h

@@ -11,8 +11,6 @@
 #define PBDRV_CONFIG_BLOCK_DEVICE                   (1)
 #define PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE          (10 * 1024)
 #define PBDRV_CONFIG_BLOCK_DEVICE_TEST              (1)
-#define PBDRV_CONFIG_BLOCK_DEVICE_TEST_SIZE         (8 * 1024)
-#define PBDRV_CONFIG_BLOCK_DEVICE_TEST_SIZE_USER    (512)
 
 #define PBDRV_CONFIG_BUTTON                         (1)
 #define PBDRV_CONFIG_BUTTON_NXT                     (1)

lib/pbio/platform/nxt/pbsysconfig.h

@@ -11,7 +11,7 @@
 #define PBSYS_CONFIG_MAIN                           (1)
 #define PBSYS_CONFIG_STORAGE                        (1)
 #define PBSYS_CONFIG_STORAGE_NUM_SLOTS              (1)
-#define PBSYS_CONFIG_STORAGE_USER_DATA_SIZE         (PBDRV_CONFIG_BLOCK_DEVICE_TEST_SIZE_USER)
+#define PBSYS_CONFIG_STORAGE_USER_DATA_SIZE         (512)
 #define PBSYS_CONFIG_STATUS_LIGHT                   (0)
 #define PBSYS_CONFIG_STATUS_LIGHT_BATTERY           (0)
 #define PBSYS_CONFIG_STATUS_LIGHT_BLUETOOTH         (0)

lib/pbio/sys/storage.c

@@ -22,62 +22,11 @@
 #include "core.h"
 #include "hmi.h"
 
-/**
- * Information about one code slot.
- *
- * A size of 0 means that this slot is not used. The offset indicates where
- * the program is stored in user storage.
- *
- * Code slots are *not* stored chronologically by slot id. Instead they are
- * stored consecutively as they are received, with the newest program last.
- *
- * If a slot is already in use and a new program should be loaded into it,
- * it is deleted by mem-moving any subsequent programs into its place, and
- * appending the new program to be last again. Since a user is typically only
- * iterating code in one slot, this is therefore usually the last stored
- * program. This means memmoves happen very little, only when needed.
- *
- */
-typedef struct {
-    uint32_t offset;
-    uint32_t size;
-} pbsys_storage_slot_info_t;
-
 /**
  * Slot at which incoming program data is currently being received.
  */
 static uint8_t incoming_slot = 0;
 
-/**
- * Map of loaded data. All data types are little-endian.
- */
-typedef struct {
-    /**
-     * End-user read-write accessible data. Everything after this is also
-     * user-readable but not writable.
-     */
-    uint8_t user_data[PBSYS_CONFIG_STORAGE_USER_DATA_SIZE];
-    /**
-     * First 8 symbols of the git hash of the firmware version used to create
-     * this data map. If this does not match the version of the running
-     * firmware, user data will be reset to 0.
-     */
-    char stored_firmware_hash[8];
-    /**
-     * System settings. Settings will be reset to defaults when the firmware
-     * version changes due to an update.
-     */
-    pbsys_storage_settings_t settings;
-    /**
-     * Size and offset info for each slot.
-     */
-    pbsys_storage_slot_info_t slot_info[PBSYS_CONFIG_STORAGE_NUM_SLOTS];
-    /**
-     * Data of the application program (code + heap).
-     */
-    uint8_t program_data[] __attribute__((aligned(sizeof(void *))));
-} pbsys_storage_data_map_t;
-
 /**
  * Map of loaded data. This is kept in memory between successive runs of the
  * end user application (like MicroPython).
@@ -399,7 +348,7 @@ static pbio_error_t pbsys_storage_deinit_process_thread(pbio_os_state_t *state,
  */
 void pbsys_storage_init(void) {
 
-    pbio_error_t err = pbdrv_block_device_get_data((uint8_t **)&map);
+    pbio_error_t err = pbdrv_block_device_get_data(&map);
 
     // Test that storage successfully loaded and matches current firmware,
     // otherwise reset storage.

lib/pbio/sys/storage_data.h

@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: MIT
+// Copyright (c) 2025 The Pybricks Authors
+
+// Data structures for non-volatile storage.
+
+// NB: Normally, pbsys code isn't referenced by drivers, however this is a
+// special case where the block device driver needs to have the actual struct
+// definition in order to ensure correct memory layout and alignment.
+
+#ifndef _PBSYS_SYS_STORAGE_DATA_H_
+#define _PBSYS_SYS_STORAGE_DATA_H_
+
+#include <stdint.h>
+
+#include <pbsys/config.h>
+#include <pbsys/storage_settings.h>
+
+/**
+ * Information about one code slot.
+ *
+ * A size of 0 means that this slot is not used. The offset indicates where
+ * the program is stored in user storage.
+ *
+ * Code slots are *not* stored chronologically by slot id. Instead they are
+ * stored consecutively as they are received, with the newest program last.
+ *
+ * If a slot is already in use and a new program should be loaded into it,
+ * it is deleted by mem-moving any subsequent programs into its place, and
+ * appending the new program to be last again. Since a user is typically only
+ * iterating code in one slot, this is therefore usually the last stored
+ * program. This means memmoves happen very little, only when needed.
+ *
+ */
+typedef struct {
+    uint32_t offset;
+    uint32_t size;
+} pbsys_storage_slot_info_t;
+
+/**
+ * Map of loaded data. All data types are little-endian.
+ */
+typedef struct {
+    /**
+     * End-user read-write accessible data. Everything after this is also
+     * user-readable but not writable.
+     */
+    uint8_t user_data[PBSYS_CONFIG_STORAGE_USER_DATA_SIZE];
+    /**
+     * First 8 symbols of the git hash of the firmware version used to create
+     * this data map. If this does not match the version of the running
+     * firmware, user data will be reset to 0.
+     */
+    char stored_firmware_hash[8];
+    /**
+     * System settings. Settings will be reset to defaults when the firmware
+     * version changes due to an update.
+     */
+    pbsys_storage_settings_t settings;
+    /**
+     * Size and offset info for each slot.
+     */
+    pbsys_storage_slot_info_t slot_info[PBSYS_CONFIG_STORAGE_NUM_SLOTS];
+    /**
+     * Data of the application program (code + heap).
+     */
+    uint8_t program_data[] __attribute__((aligned(sizeof(void *))));
+} pbsys_storage_data_map_t;
+
+#endif // _PBSYS_SYS_STORAGE_DATA_H_