Fix: Open files read only if requested #11747

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

Contributors
Elbandi sipa promag jonasschnelli laanwj MarcoFalke

Labels
Block storage