refactor: avoid possible UB from `std::distance` for `nullptr` args

l0rinc commented at 1:01 pm on December 28, 2025: contributor

Problem

Calling std::distance(nullptr, nullptr) has ambiguous status in the C++ standard iterator.requirements.general:

Iterators can also have singular values that are not associated with any sequence. Results of most expressions are undefined for singular values.

It seems to work correctly in every implementation we use, but LWG 1213 (“Meaning of valid and singular iterator underspecified”) has been Open since 2009, acknowledging that the standard’s wording on this topic is unclear.

The iterator.requirements.general states:

Iterators can also have singular values that are not associated with any sequence. Results of most expressions are undefined for singular values.

And LWG 208’s rationale explicitly confirms:

Null pointers are singular.

Therefore they cannot form a valid range required by std::distance:

Preconditions: last is reachable from first, or InputIterator meets the Cpp17RandomAccessIterator requirements and first is reachable from last.

Fix

A previous version of this PR checked both values for nullptr, the current one uses unambiguously well-defined pointer subtraction instead, which is per expr.add:

If P and Q both evaluate to null pointer values, the value is 0.

This applies on the first call before any memory is allocated, when both pointers are nullptr. Using operator- directly is simpler and avoids the ambiguity entirely.

DrahtBot added the label Refactoring on Dec 28, 2025

DrahtBot commented at 1:01 pm on December 28, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34161.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, optout21
Stale ACK	bensig

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

l0rinc marked this as ready for review on Dec 28, 2025

bensig commented at 9:44 pm on January 8, 2026: contributor

ACK b9ffd11eeca0171ac3a429aa4fc74e455a13698a tested

in src/support/allocators/pool.h:163 in b9ffd11eec

161+        size_t remaining_available_bytes{0};
162+        if (m_available_memory_it != nullptr) {
163+            assert(m_available_memory_end != nullptr);
164+            remaining_available_bytes = std::distance(m_available_memory_it, m_available_memory_end);
165+        }
166+        if (remaining_available_bytes != 0) {

optout21 commented at 1:55 pm on January 9, 2026:

Why is this line changed, style-only? Is it mandated by style guide? (In an equality check, having the const side first can avoid some errors when mistakenly ‘=’ is typed instead of ‘==’, but here there is a ‘!=’ operand…)

l0rinc commented at 2:02 pm on January 20, 2026:

I don’t mind either, I understand the advantages of Yoda conditions, but we don’t often use it here. I have reverted that line based on comments.

maflcko commented at 6:41 pm on January 19, 2026: member

The fix seems overly verbose. Why not just use operator-?

Also, is there any way to observe this UB in reality? Was there a miscompilation or a sanitizer warning?

coins: replace `std::distance` with unambiguous pointer subtraction

 Avoid calling `std::distance` on null pointers in `PoolResource::AllocateChunk`.
 Compute remaining bytes with `m_available_memory_end - m_available_memory_it` instead, which is well-defined to be `0` when both are `nullptr`.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

477c5504e0

l0rinc force-pushed on Jan 20, 2026

l0rinc commented at 2:36 pm on January 20, 2026: contributor

The fix seems overly verbose. Why not just use operator-?

After the ambiguity I found about this (e.g. https://cplusplus.github.io/LWG/issue1213), I just did the validation manually instead. But you’re right that pointer subtraction is unambiguously well-defined and safe per expr.add.

Also, is there any way to observe this UB in reality? Was there a miscompilation or a sanitizer warning?

Likely not, but I was experimenting with the allocator removal in https://github.com/l0rinc/bitcoin/pull/65/changes#diff-95c977c931cf8ed9a073043116ea5d223f6943eed5755c977e9171f7e801e3b2L158 and got nerd-sniped into this one.

l0rinc renamed this:
~~refactor: Avoid UB from `std::distance` with `nullptr` in `PoolAllocator`~~
refactor: avoid possible UB from `std::distance` for `nullptr` args
on Jan 20, 2026

maflcko commented at 4:08 pm on January 20, 2026: member

I haven’t confirmed that this really is UB, and it is probably a type of UB that can be ignored. However, the patch should be harmless.

review ACK 477c5504e05f9031449cdbf62bf329eac427cb0c 🍶

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 477c5504e05f9031449cdbf62bf329eac427cb0c 🍶
3Vo7RvygAOY2kvFNNZ4T4m6mgbSJAoFqxjtnVsLgHE8ASm9NfYjQpxuvUS4jSC0fDyS+adUVQKPMvle0jMAiIDw==

I also checked that on my system with clang and gcc, the bitcoind shasum is identical with or without this commit, for -DCMAKE_BUILD_TYPE=Release. However, I haven’t checked all architectures.

optout21 commented at 6:47 am on January 21, 2026: contributor

ACK 477c5504e05f9031449cdbf62bf329eac427cb0c

Change of a single std::distance to operator- iterator difference, which is simpler and not prone to the UB in case of null iterator.

refactor: avoid possible UB from std::distance for nullptr args #34161

Problem

Fix

Code Coverage & Benchmarks

Reviews

refactor: avoid possible UB from `std::distance` for `nullptr` args #34161