Running the while loop up to NUM_ITERS times may set iter to g_outpoints.size(), which will then lead to an out-of-bounds read.
There was an assert, which I guess tried to catch this, but the condition in the assert was wrong as well.
Fix all issues by replacing the broken assert with the internal and correct check inside std::vector::at and by limiting iter to NUM_ITERS in the while loop.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32122.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | glozow, brunoerg |
| Concept ACK | kevkevinpal |
| Stale ACK | dergoegge, instagibbs |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Concept ACK
I was able to reproduce the error using the input in #32121
I think your approach works but I was wondering
why not just modify initialize_package_rbf to use this statement
for (int i = 0; i <= NUM_ITERS; ++i)
instead of
for (int i = 0; i < NUM_ITERS; ++i)
why not just modify
initialize_package_rbfto use this statement
Why would that be better? Once an off-by-two is added in a future patch, the fuzz target once again will crash, whereas with the patch in this pull, it will work fine?
utACK fa9d9a33420c2db35e9ee7eebe5d6e475e26a8e8
This was caught in the qa-assets CI because we’re using a hardened libc++ build? Trying to understand why I haven’t seen this in my fuzzing.
This was caught in the qa-assets CI because we’re using a hardened libc++ build? Trying to understand why I haven’t seen this in my fuzzing.
Yes. This particular one doesn’t require an ABI breaking build, so I think you should be able to find it with any of (fast, extensive, debug) from https://libcxx.llvm.org/Hardening.html#mapping-between-the-hardening-modes-and-the-assertion-categories. Same for GCC: It should be found by any of (ASSERTIONS, DEBUG, DEBUG_PEDANTIC). However, I haven’t tried any of this.
utACK fa9d9a33420c2db35e9ee7eebe5d6e475e26a8e8
change looks right
127 int64_t running_vsize_total{replacement_vsize};
128
129 LOCK2(cs_main, pool.cs);
130
131- LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), NUM_ITERS)
132+ LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), NUM_ITERS - iter)
Nit: I find it a bit unintuitive to use a loop variable inside the condition here, and would prefer:
0LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), NUM_ITERS) {
1 if (iter >= NUM_ITERS) break;
2 ...
why not just modify
initialize_package_rbfto use this statementWhy would that be better? Once an off-by-two is added in a future patch, the fuzz target once again will crash, whereas with the patch in this pull, it will work fine?
ahh ok I see now that makes sense to me
