- Notifications
You must be signed in to change notification settings - Fork1.2k
Fix some errors from -fanalyzer.#2697
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:develop
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Includes a genuine bug in multicore_doorbell_claim() which seemed to use the wrong bit for the second core.
Wren6991 commentedOct 15, 2025
The SHA-256 errors are OOB reads in diff --git a/src/rp2_common/pico_sha256/sha256.c b/src/rp2_common/pico_sha256/sha256.cindex 91009c8..53dfaab 100644--- a/src/rp2_common/pico_sha256/sha256.c+++ b/src/rp2_common/pico_sha256/sha256.c@@ -75,6 +75,8 @@ int pico_sha256_start_blocking_until(pico_sha256_state_t *state, enum sha256_end return rc; }+#pragma GCC diagnostic push+#pragma GCC diagnostic ignored "-Wanalyzer-out-of-bounds" static void write_to_hardware(pico_sha256_state_t *state, const uint8_t *data, size_t data_size_bytes) { if (state->channel >= 0) { dma_channel_wait_for_finish_blocking(state->channel);@@ -111,6 +113,7 @@ static void write_to_hardware(pico_sha256_state_t *state, const uint8_t *data, s } } }+#pragma GCC diagnostic pop static void update_internal(pico_sha256_state_t *state, const uint8_t *data, size_t data_size_bytes) { assert(state->locked); ...but there is a lot of other scary buffer handling in there which we want to be analysed. |
lurch commentedOct 15, 2025
Same thing as#2667 ? |
Wren6991 commentedOct 16, 2025
Yes, same fix. |
These are from building
kitchen_sinkwith-fanalyzeron GCC 14.3.pico_time+ 1on claim bit for second core inmulticore_doorbell_claim()-- besides being OOB this also seems to just be looking at the wrong bit as it doesn't match the one used inmulticore_doorbell_unclaim()local_rng_stateininitialise_rand-- this looked like it was trying to be deliberately uninitialised but this doesn't garner much entropy for a stack variable, and it's undefined behaviourThere are some remaining errors in the SHA-256 which look pessimistic to me. It would still be good to clean that up so that people can use
-fanalyzerwhen building against the SDK.