Avoid file overwriting in fallback AllocateFileRange implementation #33164

1. 
   hebasto commented at 8:38 pm on August 9, 2025: member

   On the master branch, the fallback variant of AllocateFileRange, introduced in #1677, overwrites the file’s content. This causes issues on some systems during some “reindex” scenarios. Additionally, the recently introduced feature_reindex_init.py test is also broken on these systems.

   The affected systems include: OpenBSD, NetBSD, OmniOS, OpenIndiana.

   This PR avoids such overwriting.

   Fixes #33128 and feature_reindex_init.py test on affected systems.

2. Avoid file overwriting in fallback `AllocateFileRange` implementation 6528709c87
3. hebasto added the label Block storage on Aug 9, 2025
4. 
   DrahtBot commented at 8:38 pm on August 9, 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/33164.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #33228 (Bugfix: AllocateFileRange: Address various issues by luke-jr)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

5. in src/util/fs_helpers.cpp:209 in 6528709c87

   205@@ -206,17 +206,28 @@ void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length)
   206     if (0 == posix_fallocate(fileno(file), 0, nEndPos)) return;
   207 #endif
   208     // Fallback version
   209-    // TODO: just write one byte per block
   

   ---

   ---

   

   luke-jr commented at 12:43 pm on August 11, 2025:

   Why lose this?
6. in src/util/fs_helpers.cpp:212 in 6528709c87

   210-    static const char buf[65536] = {};
   211-    if (fseek(file, offset, SEEK_SET)) {
   212+    if (fseek(file, 0, SEEK_END)) {
   213+        return;
   214+    }
   215+    const int64_t filesize = std::ftell(file);
   

   ---

   ---

   

   luke-jr commented at 12:45 pm on August 11, 2025:

   It looks safe to mix std::ftell with C file i/o functions, but is there any reason to do so?
7. in src/util/fs_helpers.cpp:219 in 6528709c87

   218+    }
   219+    if (offset + length <= static_cast<unsigned int>(filesize)) {
   220         return;
   221     }
   222-    while (length > 0) {
   223+    unsigned int inc_size = offset + length - static_cast<unsigned int>(filesize);
   

   ---

   ---

   

   luke-jr commented at 12:46 pm on August 11, 2025:

   Probably better to use size_t or at least long

   ---

   

   luke-jr commented at 2:29 pm on August 11, 2025:

   Change of behaviour: This can allocate before offset if the file is smaller. If that’s desired behaviour, there’s no point to the offset parameter at all…?
8. in src/util/fs_helpers.cpp:222 in 6528709c87

   221     }
   222-    while (length > 0) {
   223+    unsigned int inc_size = offset + length - static_cast<unsigned int>(filesize);
   224+    if (fseek(file, filesize, SEEK_SET)) {
   225+        return;
   226+    }
   

   ---

   ---

   

   luke-jr commented at 2:30 pm on August 11, 2025:

   Redundant?
9. luke-jr changes_requested
10. 
    luke-jr commented at 2:32 pm on August 11, 2025: member

    The description for AllocateFileRange says “the range specified in the arguments will never contain live data”, so I’m not sure this is the right approach.
11. 
    cedwies commented at 4:20 pm on August 19, 2025: none

    Clarification: The doc for AllocateFileRange says: “the range specified … will never contain live data.” But in reality some call paths (during reindex on a few OSes) did pass ranges that overlapped live bytes? The old fallback then zero‑wrote inside the file and corrupted data. This PR makes the fallback defensive by appending after EOF only, so even if a caller violates the promise, we still don’t overwrite (?).

    But what about the guarantee from the doc of AllocateFileRange? Should we dismiss this guarantee? Maybe we should clarify which assumptions we can make e.g. filesize < offset (only appending at the end) or offset + length > filesize (no shrinking because if I am not mistaken SetEndOfFile / ftruncate will then shrink the file, potentially erasing live data). So we should guard against that?

Contributors
hebasto DrahtBot luke-jr cedwies

Labels
Block storage