bug: Prevent memory_reset_hww from masking flash write failures #1707
Conversation
src/memory/memory.c
Outdated
| rust_noise_generate_static_private_key(rust_util_bytes_mut( | ||
| chunk.fields.noise_static_private_key, sizeof(chunk.fields.noise_static_private_key))); | ||
| bool res = _write_chunk(CHUNK_1, chunk.bytes); | ||
| if (!_write_chunk(CHUNK_1, chunk.bytes)) { |
There was a problem hiding this comment.
I guess the original intention was to run the below BLE stuff even if this fails, so to preserve this, just assign bool res1 and bool res2 and return res1 && res2?
cedwies
force-pushed
the
cedwies/memory-reset-hww-flash-result
branch
from
55104dd to
16b777b
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/memory/memory.c
Outdated
| chunk_shared.fields.ble_identity_address[0] |= 0xc; | ||
|
|
||
| memset(&chunk_shared.fields.ble_bond_db, 0xff, sizeof(chunk_shared.fields.ble_bond_db)); | ||
| res |= _write_to_address(FLASH_SHARED_DATA_START, 0, chunk_shared.bytes); | ||
| res = res && _write_to_address(FLASH_SHARED_DATA_START, 0, chunk_shared.bytes); |
There was a problem hiding this comment.
Using res = res && _write_to_address(...) short-circuits the shared BLE reset when _write_chunk(CHUNK_1, ...) fails because res is already false. The commit message says both writes should still be attempted, but this change prevents the second write from running and leaves shared BLE data unchanged on partial failures. Consider invoking the shared write unconditionally and combining results afterward so both operations are attempted while still returning false if either fails.
Useful? React with 👍 / 👎.
memory_reset_hww combined the CHUNK_1 write result and the shared BLE chunk write using |=, so a later successful write could hide an earlier failure. Change both writes to be checked individually and return false on the first failure, making the function only report success when all flash writes succeed.
cedwies
force-pushed
the
cedwies/memory-reset-hww-flash-result
branch
from
16b777b to
42b1a4c
Compare
|
|
||
| memset(&chunk_shared.fields.ble_bond_db, 0xff, sizeof(chunk_shared.fields.ble_bond_db)); | ||
| res |= _write_to_address(FLASH_SHARED_DATA_START, 0, chunk_shared.bytes); | ||
| res = _write_to_address(FLASH_SHARED_DATA_START, 0, chunk_shared.bytes) && res; |
There was a problem hiding this comment.
relevant: https://c0x.shape-of-code.com/6.5.13.html
first operand always executes
3 participants
memory_reset_hww combined the CHUNK_1 write result and the shared BLE chunk write using |=, so a later successful write could hide an earlier failure. Change both writes to be checked individually and return false if any of the two write operations fails, but ensuring both operations are attempted.
What could go wrong in the old code:
when
_write_chunk(CHUNK_1, chunk.bytes);returnfalseand_write_to_address(FLASH_SHARED_DATA_START, 0, chunk_shared.bytes)returns true, thenbool memory_reset_hww(void)returns true due to the|=operator.