Fix: Open files read only if requested #11747

pull Elbandi wants to merge 1 commits into bitcoin:master from Elbandi:fix11745 changing 1 files +1 −3
  1. Elbandi commented at 7:41 pm on November 21, 2017: contributor
    possible fix for #11745 .
  2. sipa commented at 1:34 am on November 22, 2017: member
    utACK e1a8ec56c56161be15af1c33067918959e2666de
  3. in src/validation.cpp:3467 in e1a8ec56c5 outdated
    3463@@ -3464,7 +3464,7 @@ static FILE* OpenDiskFile(const CDiskBlockPos &pos, const char *prefix, bool fRe
    3464         return nullptr;
    3465     fs::path path = GetBlockPosFilename(pos, prefix);
    3466     fs::create_directories(path.parent_path());
    3467-    FILE* file = fsbridge::fopen(path, "rb+");
    


    promag commented at 2:14 pm on November 22, 2017:

    Isn’t the following enough?

    0FILE* file = fsbridge::fopen(path, fReadOnly ? "rb": "ab+");
    1if (!file) {
    2    LogPrintf("Unable to open file %s\n", path.string());
    3    return nullptr;
    4}
    

    Why the second attempt when it’s not read only?

    Edit: updated example after comment below.


    Elbandi commented at 3:13 pm on November 22, 2017:
    “w+” is not good because it truncates the file. If we want to merge the two fopen, the best write mode is “ab+”

    promag commented at 3:54 pm on November 22, 2017:
    Yes! Saw the manpage, but forgot to change after paste.
  4. promag commented at 2:14 pm on November 22, 2017: member
    Fix ACK.
  5. jonasschnelli commented at 7:27 pm on November 22, 2017: contributor

    utACK e1a8ec56c56161be15af1c33067918959e2666de

    Is this something that should be backported? I guess this can hardly produce corruptions in block files due to always opening in write mode? Would the open fail if one symbolic links historical block files from a read only filesystem?

  6. jonasschnelli added the label Block storage on Nov 23, 2017
  7. laanwj commented at 7:19 pm on November 23, 2017: member

    Would the open fail if one symbolic links historical block files from a read only filesystem?

    Right - this is a permissions issue, not a corruption issue, opening a file for writing is harmless if you never write to it.

    utACK anyhow. (was for previous version of this PR)

  8. Fix: Open files read only if requested 7651ff25cf
  9. Elbandi force-pushed on Nov 23, 2017
  10. Elbandi commented at 8:16 pm on November 23, 2017: contributor
    i pushed a new commit with “merge” the two fopen as @promag recommended
  11. in src/validation.cpp:3467 in 7651ff25cf
    3463@@ -3464,9 +3464,7 @@ static FILE* OpenDiskFile(const CDiskBlockPos &pos, const char *prefix, bool fRe
    3464         return nullptr;
    3465     fs::path path = GetBlockPosFilename(pos, prefix);
    3466     fs::create_directories(path.parent_path());
    3467-    FILE* file = fsbridge::fopen(path, "rb+");
    3468-    if (!file && !fReadOnly)
    3469-        file = fsbridge::fopen(path, "wb+");
    3470+    FILE* file = fsbridge::fopen(path, fReadOnly ? "rb": "ab+");
    


    laanwj commented at 8:23 pm on November 23, 2017:

    Don’t some operating systems enforce the append bit, so that you can really only append? From the man page:

    0       a+     Open for reading and appending (writing at end of file).  The file is created if it does not exist.  The initial  file  position  for
    1              reading is at the beginning of the file, but output is always appended to the end of the file.
    

    I don’t think that’s quite what we want. For example the code to reserve space pads the file, then seeks back.

  12. laanwj commented at 7:52 am on November 28, 2017: member
    I’d prefer reverting this to the previous version of the patch which didn’t use “a”.
  13. MarcoFalke commented at 4:39 pm on November 28, 2017: member

    @Elbandi Mind pushing the previous version (likely e1a8ec5)?

    0git checkout fix11745
    1git reset --hard ${commit_id_previous_version}
    2git push origin fix11745 -f
    
  14. promag commented at 6:20 pm on November 28, 2017: member
    Yes, in any case it’s preferable to limit the fix scope. Sorry for the noise, it sounded simpler.
  15. laanwj commented at 10:59 am on November 29, 2017: member
    Merged e1a8ec5 through e970396
  16. laanwj closed this on Nov 29, 2017

  17. laanwj referenced this in commit e97039605e on Nov 29, 2017
  18. schancel referenced this in commit e66c645130 on Jul 18, 2019
  19. jonspock referenced this in commit 5cda1eb6a9 on Aug 29, 2019
  20. proteanx referenced this in commit 8e716dcc11 on Sep 4, 2019
  21. PastaPastaPasta referenced this in commit 03f6828690 on Jan 17, 2020
  22. PastaPastaPasta referenced this in commit 8323f1580d on Jan 22, 2020
  23. PastaPastaPasta referenced this in commit 514b567340 on Jan 22, 2020
  24. PastaPastaPasta referenced this in commit ce22744322 on Jan 29, 2020
  25. PastaPastaPasta referenced this in commit 1ae4466622 on Jan 29, 2020
  26. PastaPastaPasta referenced this in commit 1e2b2b11d5 on Jan 29, 2020
  27. ckti referenced this in commit 98ca6ba6a8 on Mar 28, 2021
  28. MarcoFalke locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me