Skip to content

Conversation

Wren6991
Copy link
Contributor

These are from building kitchen_sink with -fanalyzer on GCC 14.3.

  • Panic on failed allocation in pico_time
  • Remove + 1 on claim bit for second core in multicore_doorbell_claim() -- besides being OOB this also seems to just be looking at the wrong bit as it doesn't match the one used in multicore_doorbell_unclaim()
  • Use a constant initialiser for local_rng_state in initialise_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 behaviour

There 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 -fanalyzer when building against the SDK.

Includes a genuine bug in multicore_doorbell_claim() which seemed to use the wrong bit for the second core.
@Wren6991 Wren6991 requested a review from kilograham October 15, 2025 12:14
@Wren6991 Wren6991 added this to the 2.2.1 milestone Oct 15, 2025
@Wren6991
Copy link
Contributor Author

The SHA-256 errors are OOB reads in write_to_hardware() from constant-sized buffers passed in through write_padding() and add_zero_bytes(). It doesn't seem to propagate the bounds on the byte counts (constant 1 and <= 4 respectively) so assumes you can read past the end. The errors can be suppressed with:

diff --git a/src/rp2_common/pico_sha256/sha256.c b/src/rp2_common/pico_sha256/sha256.c
index 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
Copy link
Contributor

lurch commented Oct 15, 2025

  • Remove + 1 on claim bit for second core in multicore_doorbell_claim() -- besides being OOB this also seems to just be looking at the wrong bit as it doesn't match the one used in multicore_doorbell_unclaim()

Same thing as #2667 ?

@Wren6991
Copy link
Contributor Author

Yes, same fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants