pbio/sys/storage_data: fix strict aliasing issues #352
Conversation
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.")
dlech
force-pushed
the
fix-block-device-test-nxt
branch
from
fbfca4c to
5023437
Compare
|
Could we leave a little more time to review, please? Is there a way to do this without including |
|
This fix helps to have a more stable REPL on the NXT, but the EV3 now suffers something like issue 2272 Went back too github hash 96cdd12 to get a usable REPL. |
We could, but we would lose the strict safety that this provides. |
|
Maybe it works to move the union to the pbsys side of things? We would probably need to change the array type from uint8_t to uint32_t though to get the right alignment in the block drivers. And when we have multiple slots, we need to make sure each slot is 4-byte aligned as well. I'll see if I can come up with something. |
|
Fwiw, we might be dropping slots at the RAM level, so if these overcomplicate things you could skip this aspect of it. |
4 participants
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.")