Deduplicate crypto libraries, saving ~49KB flash#1632
Deduplicate crypto libraries, saving ~49KB flash#1632ViezeVingertjes wants to merge 2 commits intomeshcore-dev:devfrom
Conversation
Fixes ed25519_verify() stack overflow by making large locals static, removing the need for rweather's Ed25519::verify() workaround. Vendors only the AES128+SHA256 subset of rweather/Crypto instead of all 37 files.
weebl2000
left a comment
weebl2000
left a comment
There was a problem hiding this comment.
Nice improvement — vendoring the minimal subset is clean and the stack savings are real (~1.8KB moved to BSS). One concern about the static locals in ge.c.
| static signed char aslide[256]; | ||
| static signed char bslide[256]; | ||
| static ge_cached Ai[8]; /* A,3A,5A,7A,9A,11A,13A,15A */ |
There was a problem hiding this comment.
Making these static saves ~1.8KB of stack which fixes the overflow, but it also makes ge_double_scalarmult_vartime() non-reentrant. If ed25519_verify() is ever called concurrently from multiple FreeRTOS tasks, these shared buffers will be silently corrupted.
Currently this looks safe — Identity::verify() is only called from the packet receive path in Mesh.cpp — but it's worth documenting so future maintainers don't get bitten.
| static signed char aslide[256]; | |
| static signed char bslide[256]; | |
| static ge_cached Ai[8]; /* A,3A,5A,7A,9A,11A,13A,15A */ | |
| static signed char aslide[256]; /* static: saves ~1.8KB stack; NOT reentrant */ | |
| static signed char bslide[256]; | |
| static ge_cached Ai[8]; /* A,3A,5A,7A,9A,11A,13A,15A */ |
There was a problem hiding this comment.
What do you thnk about an Assert? just in case its not read... (guilty of it sometimes)
There was a problem hiding this comment.
Nice, the assert makes the constraint very visible. Two minor notes:
assert()is typically compiled out in release builds (NDEBUG), so this is really a development-time guard. That's fine for this purpose — the comment does the heavy lifting for future readers.- The
static volatile boolcheck + set isn't atomic, so there's a theoretical TOCTOU race. In practice this doesn't matter — ESP32 FreeRTOS tasks on a single core won't preempt mid-statement, and even on dual-core the window is vanishingly small. Just mentioning it for completeness.
Overall this looks good to me.
2 participants
Fixes a stack overflow in ed25519_verify() by making large local variables static, removing the need for rweather's Ed25519::verify() workaround.
Only vendors the AES128+SHA256 subset of rweather/Crypto instead of all 37 files.
No issues observed so far. Additional testing and confirmation are welcome. This change frees up stack space for future improvements and improves support for resource-limited devices.