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); +}