rpc: Write authcookie atomically #11131

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2017_08_atomic_cookie changing 2 files +16 −7

laanwj commented at 10:58 AM on August 25, 2017: member
Use POSIX rename atomicity at the bitcoind side to create a working cookie atomically:
- Write .cookie.tmp, close file
- Rename .cookie.tmp to .cookie
This avoids clients reading invalid/partial cookies as in #11129. As such, this is an alternative to that PR.
laanwj added the label RPC/REST/ZMQ on Aug 25, 2017
MarcoFalke commented at 11:40 AM on August 25, 2017: member

utACK 43e7dcb
in src/rpc/protocol.cpp:70 in 43e7dcbcf0 outdated
```
  68 | @@ -66,9 +69,13 @@ static const std::string COOKIEAUTH_USER = "__cookie__";
  69 |  /** Default name for auth cookie file */
  70 |  static const std::string COOKIEAUTH_FILE = ".cookie";
  71 |  
  72 | -fs::path GetAuthCookieFile()
  73 | +static fs::path GetAuthCookieFile(bool temp)
```
promag commented at 12:57 PM on August 25, 2017:

Append the suffix in GenerateAuthCookie() and there is no need to call GetAuthCookieFile() twice.

laanwj commented at 1:16 PM on August 25, 2017:

Yeah, well, find a fool proof and portable way to append to a fs::path (without adding path components!) and get back me. After browsing stack overflow and boost documentation for a while, I deemed it easier and safer to just do it where the filename is constructed initially.

Edit: oh, and if you want to hear something else funny, calling path::replace_extension will replace .cookie with .tmp. ARg.

promag commented at 1:24 PM on August 25, 2017:

I also saw all that, but with GetDataDir() / fs::unique_path() none of ths is really necessary.
in src/rpc/protocol.cpp:19 in 43e7dcbcf0 outdated
```
  14 | @@ -15,6 +15,9 @@
  15 |  #include <stdint.h>
  16 |  #include <fstream>
  17 |  
  18 | +/** Get name of RPC authentication cookie file */
  19 | +static fs::path GetAuthCookieFile(bool temp=false);
```
promag commented at 12:58 PM on August 25, 2017:

Remove forward declaration?

laanwj commented at 1:17 PM on August 25, 2017:

Is it only used after the declaration? To minimize diff I didn't want to move the function around and had some compile issues, but that might have been for another reason, will try again.

MarcoFalke commented at 2:17 PM on August 25, 2017:

I thought we always put the documentation above the declaration, but whatever, no need to change it back now.
promag commented at 1:10 PM on August 25, 2017: member

Another option is to use fs::unique_path.

Edit: Concept ACK!
laanwj commented at 1:14 PM on August 25, 2017: member

Another option is to use fs::unique_path.

I wasn't sure that would work in all cases - using a temporary file would potentially move over filesystem boundaries, right? Cross-filesystem move is not atomic IIRC.
promag commented at 1:19 PM on August 25, 2017: member

I meant GetDataDir() / fs::unique_path().
laanwj commented at 1:24 PM on August 25, 2017: member

I meant GetDataDir() / fs::unique_path().

It's possible to place the cookie file outside of the data directory with an option. Also my gut feeling says it's better to create files with deterministic names in the data directory, what if e.g. boost accidentally generates a name used as a wallet name :) I don't see any advantages.
promag commented at 1:28 PM on August 25, 2017: member

what if e.g. boost accidentally generates a name used as a wallet name :)

I see you take serious precautions 😄

In the documentation there is http://www.boost.org/doc/libs/1_53_0/libs/filesystem/doc/reference.html#path-concatenation.
laanwj force-pushed on Aug 25, 2017
in src/rpc/protocol.cpp:99 in 43e7dcbcf0 outdated
```
  99 | -        LogPrintf("Unable to open cookie authentication file %s for writing\n", filepath.string());
 100 | +        LogPrintf("Unable to open cookie authentication file %s for writing\n", filepath_tmp.string());
 101 |          return false;
 102 |      }
 103 |      file << cookie;
 104 |      file.close();
```
promag commented at 1:32 PM on August 25, 2017:

I've not tested but here says:

A common misconception is that calling close will necessarily perform an implicit fsync.

From the docs there is no explicit mention to flush, only:

Any pending output sequence is written to the file.

promag commented at 1:34 PM on August 25, 2017:

In other words, call file.flush() before close?

laanwj commented at 1:35 PM on August 25, 2017:

You're having another misconception there: fsync is not necessary for other processes to see the result! Only to make sure that the data hits disk, in case of crashes etc. Which is not relevant here.

laanwj commented at 1:45 PM on August 25, 2017:

Let's ask @theuni just in case.

sipa commented at 5:40 PM on August 25, 2017:

I think the only concern is that libc may buffer the write as long as the file is open, in which case a flush would be useful to make it visible to the OS (and thus other processes). However, as you're closing the file anyway, I don't believe that matters.
laanwj commented at 1:33 PM on August 25, 2017: member

http://www.boost.org/doc/libs/1_53_0/libs/filesystem/doc/reference.html#path-concatenation.

Yes, but as I understoof, there is difference between boost_filesystem versions there. I don't really have a lot of time to dive into that, and I don't want this to come back with some strange compile error, and this works so...

rpc: Write authcookie atomically

Use POSIX rename atomicity at the `bitcoind` side to create a working
cookie atomically:

- Write `.cookie.tmp`, close file
- Rename `.cookie.tmp` to `.cookie`

This avoids clients reading invalid/partial cookies as in #11129.

82dd7195e1

laanwj force-pushed on Aug 25, 2017
MarcoFalke added this to the milestone 0.15.1 on Aug 25, 2017
MarcoFalke added the label Needs backport on Aug 25, 2017
gmaxwell commented at 7:58 PM on August 25, 2017: contributor

Concept ACK
meshcollider commented at 12:40 PM on August 26, 2017: contributor

utACK https://github.com/bitcoin/bitcoin/pull/11131/commits/82dd7195e1fb943f9cd45a48188f9678219c0206

I don't want this to come back with some strange compile error, and this works so...

Agreed, this seems fine to me
MarcoFalke requested review from theuni on Aug 27, 2017
sipa commented at 6:54 PM on August 27, 2017: member

utACK 82dd7195e1fb943f9cd45a48188f9678219c0206
laanwj merged this on Aug 28, 2017
laanwj closed this on Aug 28, 2017
laanwj referenced this in commit c7229ac36e on Aug 28, 2017
MarcoFalke referenced this in commit b278a43646 on Oct 3, 2017
MarcoFalke removed the label Needs backport on Oct 4, 2017
Fabcien referenced this in commit e26ef4b479 on Aug 30, 2019
PastaPastaPasta referenced this in commit 2f12d50751 on Sep 19, 2019
PastaPastaPasta referenced this in commit 900c6ae316 on Sep 23, 2019
PastaPastaPasta referenced this in commit 126034e9dc on Sep 24, 2019
codablock referenced this in commit 6ec77c0a71 on Sep 25, 2019
jonspock referenced this in commit a2c23f7b69 on Dec 8, 2019
jonspock referenced this in commit c314044f4c on Dec 8, 2019
jonspock referenced this in commit 9e26ac103a on Dec 8, 2019
proteanx referenced this in commit 70f7927126 on Dec 12, 2019
barrystyle referenced this in commit e856e38ff4 on Jan 22, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Review Requested
theuni

Labels

RPC/REST/ZMQ

Milestone
0.15.2

Linked (view graph)

#11129 [qa] util: Poll cookie file size before reading