From dfdca1b50020322b5ada735a428d674eb5c27c04 Mon Sep 17 00:00:00 2001 From: bluezr Date: Thu, 6 Aug 2026 10:03:59 -0700 Subject: [PATCH] wallet, utils, headersdb: put null guards above the dereference 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. --- src/headersdb_file.c | 5 ++++- src/utils.c | 9 +++++++++ src/wallet.c | 14 ++++++++++---- test/unittester.c | 4 ++++ test/utils_tests.c | 20 ++++++++++++++++++++ test/wallet_tests.c | 37 +++++++++++++++++++++++++++++++++++++ 6 files changed, 84 insertions(+), 5 deletions(-) diff --git a/src/headersdb_file.c b/src/headersdb_file.c index a851c413c..b648c20b6 100644 --- a/src/headersdb_file.c +++ b/src/headersdb_file.c @@ -439,7 +439,10 @@ dogecoin_blockindex * dogecoin_headers_db_connect_hdr(dogecoin_headers_db* db, s // keep them only on-disk dogecoin_blockindex *scan_tip = db->chaintip; unsigned int i; - for (i = 0; i < db->max_hdr_in_mem + 1; i++) + /* scan_tip was dereferenced below before the `scan_tip &&` check + further down had a chance to run. chaintip is NULL until the + first header is connected. */ + for (i = 0; scan_tip && i < db->max_hdr_in_mem + 1; i++) { if (scan_tip->prev) { scan_tip = scan_tip->prev; diff --git a/src/utils.c b/src/utils.c index f0af3f94d..5441c9b89 100644 --- a/src/utils.c +++ b/src/utils.c @@ -594,7 +594,11 @@ void print_header(char* filepath) { if ((fptr = fopen(filename, "r")) == NULL) { + /* The error branch used to fall through: print_image() then ran fgets() + on a NULL FILE*, and fclose(NULL) followed it. Opening a file that is + not there is ordinary, so this crashed on ordinary input. */ fprintf(stderr, "error opening %s\n", filename); + return; } print_image(fptr); @@ -610,6 +614,11 @@ void print_image(FILE* fptr) #ifndef USE_OPTEE // OPTEE has no filesystem or console char read_string[MAX_LEN]; + /* Exported, so a caller outside this file can reach it with NULL too. */ + if (!fptr) { + return; + } + while (fgets(read_string, sizeof(read_string), fptr) != NULL) printf("%s", read_string); #else diff --git a/src/wallet.c b/src/wallet.c index 6c567d3f2..16de7c9ad 100644 --- a/src/wallet.c +++ b/src/wallet.c @@ -1722,6 +1722,9 @@ int64_t dogecoin_wallet_get_debit_txi(dogecoin_wallet *wallet, const dogecoin_tx int64_t dogecoin_wallet_get_debit_tx(dogecoin_wallet *wallet, const dogecoin_tx *tx) { unsigned int i; int64_t debit = 0; + if (!wallet || !tx) { + return 0; + } if (tx->vin) { for (i = 0; i < tx->vin->len; i++) { dogecoin_tx_in* tx_in= vector_idx(tx->vin, i); @@ -1735,14 +1738,17 @@ int64_t dogecoin_wallet_get_debit_tx(dogecoin_wallet *wallet, const dogecoin_tx dogecoin_bool dogecoin_wallet_is_from_me(dogecoin_wallet *wallet, const dogecoin_tx *tx) { - if (dogecoin_wallet_get_debit_tx(wallet, tx) > 0) { - return true; - } - + /* This guard used to sit below the get_debit_tx() call, which dereferences + tx immediately, so a NULL tx crashed four lines before the check that + exists to catch it. */ if (!wallet || !tx || !tx->vin) { return false; } + if (dogecoin_wallet_get_debit_tx(wallet, tx) > 0) { + return true; + } + for (unsigned int i = 0; i < tx->vin->len; i++) { dogecoin_tx_in* tx_in = vector_idx(tx->vin, i); if (!tx_in) { diff --git a/test/unittester.c b/test/unittester.c index 96dd35fae..dc9a606c5 100644 --- a/test/unittester.c +++ b/test/unittester.c @@ -97,6 +97,7 @@ extern void test_invalid_tx_deser(); extern void test_tx_sign(); extern void test_scripts(); extern void test_utils(); +extern void test_utils_null_file_guards(); extern void test_vector(); extern void test_qr(); @@ -131,6 +132,7 @@ extern void test_examples(); #ifdef WITH_WALLET extern void test_wallet_basics(); extern void test_wallet(); +extern void test_wallet_null_tx_guards(); extern void test_wallet_malformed_reclen(); extern void test_wallet_reorg_utxo_update(); extern void test_wallet_utxo_idx_not_reused(); @@ -232,6 +234,7 @@ int main() u_run_test(test_script_parse); u_run_test(test_script_op_codeseperator); u_run_test(test_utils); + u_run_test(test_utils_null_file_guards); u_run_test(test_vector); u_run_test(test_qr); @@ -266,6 +269,7 @@ int main() #ifdef WITH_WALLET u_run_test(test_wallet_basics); u_run_test(test_wallet); + u_run_test(test_wallet_null_tx_guards); u_run_test(test_wallet_malformed_reclen); u_run_test(test_wallet_reorg_utxo_update); u_run_test(test_wallet_utxo_idx_not_reused); diff --git a/test/utils_tests.c b/test/utils_tests.c index af2fc90ea..4c7f6d207 100644 --- a/test/utils_tests.c +++ b/test/utils_tests.c @@ -132,3 +132,23 @@ void test_dit() debug_print("%s", "DIT test: disabled (DIT not supported)\n"); } } + + +/* + * 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. print_image() is LIBDOGECOIN_API, so it is + * reachable with NULL from outside this file as well. + */ +void test_utils_null_file_guards() +{ + /* Must return quietly rather than dereferencing the NULL FILE*. */ + print_image(NULL); + + /* A path that cannot be opened must not crash. */ + print_header("this-path-does-not-exist-libdogecoin-test"); + + /* NULL path was already guarded; assert it stays that way. */ + print_header(NULL); +} diff --git a/test/wallet_tests.c b/test/wallet_tests.c index 89a812525..6e1a963af 100644 --- a/test/wallet_tests.c +++ b/test/wallet_tests.c @@ -26,6 +26,7 @@ static const char *wallettmpfile = "/tmp/dummy"; #include #include #include +#include #include #include #if !defined(_WIN32) @@ -559,3 +560,39 @@ void test_wallet_balance_accounts_for_spends() { dogecoin_wallet_free(wallet); remove_all_utxos(); } + + +/* + * Null guards that sat below the dereference they were meant to guard. + * + * dogecoin_wallet_is_from_me() called dogecoin_wallet_get_debit_tx() first, + * and that function opens with `if (tx->vin)`. The `!wallet || !tx` check came + * four lines later, so a NULL tx crashed before reaching it. Both functions are + * LIBDOGECOIN_API, and rest.c passes wtx->tx to is_from_me without checking it + * (wallet.c:954 does check, which is why this survived). + * + * Without the fix these dereference NULL rather than failing an assertion. + */ +void test_wallet_null_tx_guards() +{ + dogecoin_wallet* wallet = dogecoin_wallet_new(&dogecoin_chainparams_main); + u_assert_true(wallet != NULL); + + /* The call that used to crash. */ + u_assert_int_eq((int)dogecoin_wallet_is_from_me(wallet, NULL), 0); + + /* The function underneath it, reachable directly -- rest.c:184 calls it. */ + u_assert_true(dogecoin_wallet_get_debit_tx(wallet, NULL) == 0); + + /* A NULL wallet must be refused too, not just a NULL tx. */ + u_assert_int_eq((int)dogecoin_wallet_is_from_me(NULL, NULL), 0); + u_assert_true(dogecoin_wallet_get_debit_tx(NULL, NULL) == 0); + + /* A well-formed but empty tx still answers false rather than crashing. */ + dogecoin_tx* tx = dogecoin_tx_new(); + u_assert_true(tx != NULL); + u_assert_int_eq((int)dogecoin_wallet_is_from_me(wallet, tx), 0); + dogecoin_tx_free(tx); + + dogecoin_wallet_free(wallet); +}