pico-sdk icon indicating copy to clipboard operation
pico-sdk copied to clipboard

Fix some errors from -fanalyzer.

Open Wren6991 opened this issue 2 months ago • 3 comments

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.

Wren6991 avatar Oct 15 '25 12:10 Wren6991

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.

Wren6991 avatar Oct 15 '25 12:10 Wren6991

  • 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 ?

lurch avatar Oct 15 '25 13:10 lurch

Yes, same fix.

Wren6991 avatar Oct 16 '25 08:10 Wren6991