Public commit record
68/100 · Adequate
What the developer wrote
Authored by Salvatore Ingala
Remove initial search for sentinel
It is redundant, it's simpler to just exit when the sentinel is
found.
It was also making clang static analyzer detect a false positive
out of bounds read.
✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context
What changed, and why it matters
This commit is a small code cleanup in the Ledger Bitcoin app. It removes an initial loop that counted commands and instead checks for the end marker directly while processing. The author says the old loop was unnecessary and caused a harmless false-positive warning in a static analysis tool. There is no indication this fixes a real security bug or changes app behavior in a user-visible way.
Recommended action
No security action required. Treat as a normal refactoring/cleanup commit. If reviewing, verify that all callers still initialize command lists with a CMD_CODE_END terminator and that node->step is never advanced past a valid sentinel.
Security signals we found
01
Commit message explicitly frames the change as removing a redundant scan and a static-analysis false positive, not as a security fix.
02
No change to parsing, cryptography, authorization, or transaction semantics.
03
Functional behavior for valid command lists is equivalent before and after the patch.
Evidence from the diff
In src/handler/lib/policy.c, process_generic_node previously did a pre-scan of the commands array to count entries up to the CMD_CODE_END sentinel, then used that count to decide whether processing was complete. The patch removes that pre-scan and instead reads commands[node->step].code directly, returning 1 immediately if it is CMD_CODE_END. The switch body is otherwise unchanged (apart from indentation). This is functionally equivalent for well-formed command lists and eliminates a clang static analyzer false positive about a potential out-of-bounds read. No bounds check on node->step is added or removed; the previous n_commands check is simply replaced by the sentinel check.
Changed components
src/handler/lib/policy.cprocess_generic_node functionInspect captured patch +99 / −105
diff --git a/src/handler/lib/policy.c b/src/handler/lib/policy.c
index 21ee89d..d12ec62 100644
--- a/src/handler/lib/policy.c
+++ b/src/handler/lib/policy.c
@@ -592,123 +592,117 @@ __attribute__((warn_unused_result)) static int process_generic_node(policy_parse
const generic_processor_command_t *commands = (const generic_processor_command_t *) arg;
- size_t n_commands = 0;
- while (commands[n_commands].code != CMD_CODE_END) ++n_commands;
-
- if (node->step > n_commands) {
- return WITH_ERROR(-1, "Inconsistent state");
- } else if (node->step == n_commands) {
- return 1;
- } else {
- uint8_t cmd_code = commands[node->step].code;
- uint8_t cmd_data = commands[node->step].data;
+ uint8_t cmd_code = commands[node->step].code;
+ if (cmd_code == CMD_CODE_END) {
+ return 1; // reached the end of the command list
+ }
- switch (cmd_code) {
- case CMD_CODE_OP: {
- update_output_u8(state, cmd_data);
- break;
- }
- case CMD_CODE_OP_V: {
- update_output_op_v(state, cmd_data);
- break;
- }
- case CMD_CODE_PUSH_PK: {
- const policy_node_with_key_t *policy =
- (const policy_node_with_key_t *) node->policy_node;
- uint8_t compressed_pubkey[33];
- if (-1 == get_derived_pubkey(state->dispatcher_context,
- state->wdi,
- r_policy_node_keyexpr(&policy->key),
- compressed_pubkey)) {
- return -1;
- }
+ uint8_t cmd_data = commands[node->step].data;
- if (!state->is_taproot) {
- update_output_u8(state, 33); // PUSH 33 bytes
- update_output(state, compressed_pubkey, 33);
- } else {
- // x-only pubkey if within taproot
- update_output_u8(state, 32); // PUSH 32 bytes
- update_output(state, compressed_pubkey + 1, 32);
- }
- break;
+ switch (cmd_code) {
+ case CMD_CODE_OP: {
+ update_output_u8(state, cmd_data);
+ break;
+ }
+ case CMD_CODE_OP_V: {
+ update_output_op_v(state, cmd_data);
+ break;
+ }
+ case CMD_CODE_PUSH_PK: {
+ const policy_node_with_key_t *policy =
+ (const policy_node_with_key_t *) node->policy_node;
+ uint8_t compressed_pubkey[33];
+ if (-1 == get_derived_pubkey(state->dispatcher_context,
+ state->wdi,
+ r_policy_node_keyexpr(&policy->key),
+ compressed_pubkey)) {
+ return -1;
}
- case CMD_CODE_PUSH_PKH: {
- const policy_node_with_key_t *policy =
- (const policy_node_with_key_t *) node->policy_node;
- uint8_t compressed_pubkey[33];
- if (-1 == get_derived_pubkey(state->dispatcher_context,
- state->wdi,
- r_policy_node_keyexpr(&policy->key),
- compressed_pubkey)) {
- return -1;
- }
- if (!state->is_taproot) {
- crypto_hash160(compressed_pubkey, 33, compressed_pubkey); // reuse memory
- } else {
- // x-only pubkey if within taproot
- crypto_hash160(compressed_pubkey + 1, 32, compressed_pubkey); // reuse memory
- }
- update_output_u8(state, 20); // PUSH 20 bytes
- update_output(state, compressed_pubkey, 20);
- break;
- }
- case CMD_CODE_PUSH_UINT32: {
- const policy_node_with_uint32_t *policy =
- (const policy_node_with_uint32_t *) node->policy_node;
- update_output_push_u32(state, policy->n);
- break;
- }
- case CMD_CODE_PUSH_HASH20: {
- const policy_node_with_hash_160_t *policy =
- (const policy_node_with_hash_160_t *) node->policy_node;
- update_output_u8(state, 20);
- update_output(state, policy->h, 20);
- break;
+ if (!state->is_taproot) {
+ update_output_u8(state, 33); // PUSH 33 bytes
+ update_output(state, compressed_pubkey, 33);
+ } else {
+ // x-only pubkey if within taproot
+ update_output_u8(state, 32); // PUSH 32 bytes
+ update_output(state, compressed_pubkey + 1, 32);
}
- case CMD_CODE_PUSH_HASH32: {
- const policy_node_with_hash_256_t *policy =
- (const policy_node_with_hash_256_t *) node->policy_node;
- update_output_u8(state, 32);
- update_output(state, policy->h, 32);
- break;
+ break;
+ }
+ case CMD_CODE_PUSH_PKH: {
+ const policy_node_with_key_t *policy =
+ (const policy_node_with_key_t *) node->policy_node;
+ uint8_t compressed_pubkey[33];
+ if (-1 == get_derived_pubkey(state->dispatcher_context,
+ state->wdi,
+ r_policy_node_keyexpr(&policy->key),
+ compressed_pubkey)) {
+ return -1;
}
- case CMD_CODE_PROCESS_CHILD: {
- const policy_node_with_scripts_t *policy =
- (const policy_node_with_scripts_t *) node->policy_node;
- if (0 > state_stack_push(state, r_policy_node(&policy->scripts[cmd_data]), 0)) {
- return -1;
- }
- break;
+ if (!state->is_taproot) {
+ crypto_hash160(compressed_pubkey, 33, compressed_pubkey); // reuse memory
+ } else {
+ // x-only pubkey if within taproot
+ crypto_hash160(compressed_pubkey + 1, 32, compressed_pubkey); // reuse memory
}
- case CMD_CODE_PROCESS_CHILD_V: {
- const policy_node_with_scripts_t *policy =
- (const policy_node_with_scripts_t *) node->policy_node;
- if (0 > state_stack_push(state,
- r_policy_node(&policy->scripts[cmd_data]),
- node->flags)) {
- return -1;
- }
- break;
+
+ update_output_u8(state, 20); // PUSH 20 bytes
+ update_output(state, compressed_pubkey, 20);
+ break;
+ }
+ case CMD_CODE_PUSH_UINT32: {
+ const policy_node_with_uint32_t *policy =
+ (const policy_node_with_uint32_t *) node->policy_node;
+ update_output_push_u32(state, policy->n);
+ break;
+ }
+ case CMD_CODE_PUSH_HASH20: {
+ const policy_node_with_hash_160_t *policy =
+ (const policy_node_with_hash_160_t *) node->policy_node;
+ update_output_u8(state, 20);
+ update_output(state, policy->h, 20);
+ break;
+ }
+ case CMD_CODE_PUSH_HASH32: {
+ const policy_node_with_hash_256_t *policy =
+ (const policy_node_with_hash_256_t *) node->policy_node;
+ update_output_u8(state, 32);
+ update_output(state, policy->h, 32);
+ break;
+ }
+ case CMD_CODE_PROCESS_CHILD: {
+ const policy_node_with_scripts_t *policy =
+ (const policy_node_with_scripts_t *) node->policy_node;
+ if (0 > state_stack_push(state, r_policy_node(&policy->scripts[cmd_data]), 0)) {
+ return -1;
}
- case CMD_CODE_PROCESS_CHILD_VV: {
- const policy_node_with_scripts_t *policy =
- (const policy_node_with_scripts_t *) node->policy_node;
- if (0 > state_stack_push(state,
- r_policy_node(&policy->scripts[cmd_data]),
- node->flags | PROCESSOR_FLAG_V)) {
- return -1;
- }
- break;
+ break;
+ }
+ case CMD_CODE_PROCESS_CHILD_V: {
+ const policy_node_with_scripts_t *policy =
+ (const policy_node_with_scripts_t *) node->policy_node;
+ if (0 >
+ state_stack_push(state, r_policy_node(&policy->scripts[cmd_data]), node->flags)) {
+ return -1;
}
- default:
- PRINTF("Unexpected command code: %d\n", cmd_code);
+ break;
+ }
+ case CMD_CODE_PROCESS_CHILD_VV: {
+ const policy_node_with_scripts_t *policy =
+ (const policy_node_with_scripts_t *) node->policy_node;
+ if (0 > state_stack_push(state,
+ r_policy_node(&policy->scripts[cmd_data]),
+ node->flags | PROCESSOR_FLAG_V)) {
return -1;
+ }
+ break;
}
- ++node->step;
- return 0;
+ default:
+ PRINTF("Unexpected command code: %d\n", cmd_code);
+ return -1;
}
+ ++node->step;
+ return 0;
}
__attribute__((warn_unused_result)) static int process_pkh_wpkh_node(policy_parser_state_t *state,
Risk score
Our methodology →Why this scored 11/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.