Fix intermittent RadioLib static SPI buffer overflow#2708
Conversation
When RADIOLIB_STATIC_ONLY=1 is set, RadioLib's SPItransferStream() allocates two fixed-size stack buffers (buffOut and buffIn) of RADIOLIB_STATIC_ARRAY_SIZE bytes each, instead of heap-allocating exactly the right size. The default value of RADIOLIB_STATIC_ARRAY_SIZE is 256. When receiving a maximum-size LoRa packet (255 bytes, equal to MAX_TRANS_UNIT), SX126x::readBuffer() passes a 3-byte SPI command header (CMD_READ_BUFFER + offset + NOP) plus 255 bytes of payload to SPItransferStream(), for a total buffLen of 258 bytes. This overflows the 256-byte stack buffers by 2 bytes, corrupting adjacent locals and occasionally the stack canary, triggering __stack_chk_fail. The overflow is small (2 bytes on the read path, 1 byte on the write path), so it only intermittently reaches the stack canary depending on compiler-generated stack frame layout. Set RADIOLIB_STATIC_ARRAY_SIZE=260 to eliminate the overflow, with 2 bytes of margin on the read path (258 < 260). The value is placed in [arduino_base] so it applies to all target platforms.
|
Howdy! Thanks for the PR! If the default buffer size allocated by radiolib is too small, I'm thinking this issue should be opened directly in the radiolib repo so the default can be increased. That way we don't need to make changes for an upstream issue, which is likely going to affect other users of the library... |
Thank you for the suggestion. I'll try to address this issue in the radiolib repo, but they may have a different view on that. The default is not enough only for radios that use stream-type SPI (SX126x, SX128x, LR11x0) and is enough for traditional register-based chips (CC1101, nRF24, Si4432, etc.). Also, RADIOLIB_STATIC_ONLY=1 isn't the default option. As a result, the radiolib maintainers may think that if you enable RADIOLIB_STATIC_ONLY, then you have to properly configure RADIOLIB_STATIC_ARRAY_SIZE depending on your radio chip and protocol, and the default is just the minimum that is suitable for some cases but not others. Anyway, I'll propose the patch to radiolib and see what the response will be. |
|
@liamcottle it seems already fixed by commit jgromes/RadioLib@b28aa2b in RadioLib 7.7+, but the MeshCore firmware currently sticks to 7.6 |
2 participants
When RADIOLIB_STATIC_ONLY=1 is set, RadioLib's SPItransferStream() allocates two fixed-size stack buffers (buffOut and buffIn) of RADIOLIB_STATIC_ARRAY_SIZE bytes each, instead of heap-allocating exactly the right size.
The default value of RADIOLIB_STATIC_ARRAY_SIZE is 256. When receiving a maximum-size LoRa packet (255 bytes, equal to MAX_TRANS_UNIT), SX126x::readBuffer() passes a 3-byte SPI command header (CMD_READ_BUFFER + offset + NOP) plus 255 bytes of payload to SPItransferStream(), for a total buffLen of 258 bytes. This overflows the 256-byte stack buffers by 2 bytes, corrupting adjacent locals and occasionally the stack canary, triggering __stack_chk_fail.
The overflow is small (2 bytes on the read path, 1 byte on the write path), so it only intermittently reaches the stack canary depending on compiler-generated stack frame layout.
Set RADIOLIB_STATIC_ARRAY_SIZE=260 to eliminate the overflow, with 2 bytes of margin on the read path (258 < 260). The value is placed in [arduino_base] so it applies to all target platforms.