wallet, utils, headersdb: put null guards above the dereference - #404
Open
xanimo wants to merge 1 commit into
Open
wallet, utils, headersdb: put null guards above the dereference#404xanimo wants to merge 1 commit into
xanimo wants to merge 1 commit into
Conversation
Three places check for NULL after already dereferencing the thing they
are checking. cppcheck reports all three (nullPointerRedundantCheck and
ctunullpointer); its message is exact -- either the condition is
redundant or the dereference above it can be NULL.
dogecoin_wallet_is_from_me() called dogecoin_wallet_get_debit_tx() and
only then checked `!wallet || !tx || !tx->vin`. get_debit_tx() opens with
`if (tx->vin)`, so a NULL tx crashed four lines before the guard that
exists to catch it. Both functions are LIBDOGECOIN_API. rest.c:110 and
rest.c:170 pass wtx->tx straight in without checking it; wallet.c:954
does check, which is why this went unnoticed. The guard moves above the
call, and get_debit_tx() gets its own, since it is exported and rest.c:184
calls it directly.
print_header() ignored a failed fopen. The error branch printed a message
and fell through to print_image(), which ran fgets() on a NULL FILE*, and
then to fclose(NULL). Opening a file that is not there is ordinary input,
so this crashed on ordinary input. Returns on failure now, and
print_image() -- also exported -- rejects a NULL FILE* of its own accord.
headersdb_file.c took `scan_tip = db->chaintip` and dereferenced
`scan_tip->prev` at the top of the loop, while the `scan_tip &&` test sat
further down the body. chaintip is NULL until the first header connects.
The loop condition now carries the check.
Verified the tests fail without each fix rather than merely passing with
it. Reverting the wallet guard alone gives
AddressSanitizer: SEGV on unknown address 0x000000000008
which is the offset of tx->vin, and reverting the utils guard alone gives
AddressSanitizer: SEGV on unknown address 0x000000000000
Restored, 82/82 and clean under ASan.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three places check for NULL after already dereferencing the thing they're checking. cppcheck reports all three, and its wording is exact: either the condition is redundant, or the dereference above it can be NULL. It's the latter in all three cases.
Two are certain crashes on ordinary input.
1.
dogecoin_wallet_is_from_me— guard four lines too lateget_debit_txopens withif (tx->vin), so a NULLtxcrashes before reaching the guard.Both functions are
LIBDOGECOIN_API.rest.c:110andrest.c:170passwtx->txin without checking it, whilewallet.c:954guards properly — which is why this survived.get_debit_txgets its own guard too, sincerest.c:184calls it directly.2.
print_header— falls through a failedfopenA file that isn't there is ordinary input, so this crashes on ordinary input.
print_imageis exported too, so it now rejects a NULLFILE*on its own.3.
headersdb_file.c—scan_tipused before its checkscan_tip = db->chaintipthenscan_tip->prevat the top of the loop, while thescan_tip &&test sits further down the body.chaintipis NULL until the first header connects. The loop condition now carries the check.Verified, not assumed
Each fix was reverted individually to confirm the new tests catch a real crash rather than merely passing:
AddressSanitizer: SEGV on unknown address 0x000000000008— the offset oftx->vinAddressSanitizer: SEGV on unknown address 0x000000000000Restored: 82/82, clean under ASan.
Found while triaging the cppcheck backlog. Worth noting this is the same shape as three other things in this codebase: a guard that exists, looks right, and doesn't run —
assertunderNDEBUGin #382 andqr.c(#401), and now a check placed below its own dereference. Suggests the guards need to be reachable, not just present.