rpc: Write authcookie atomically #11131

laanwj · 2017-08-25T10:58:51Z

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.

maflcko · 2017-08-25T11:40:35Z

utACK 43e7dcb

promag

Another option is to use fs::unique_path.

Edit: Concept ACK!

promag · 2017-08-25T12:57:35Z

src/rpc/protocol.cpp

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

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.

I also saw all that, but with GetDataDir() / fs::unique_path() none of ths is really necessary.

promag · 2017-08-25T12:58:33Z

src/rpc/protocol.cpp

Remove forward declaration?

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.

I thought we always put the documentation above the declaration, but whatever, no need to change it back now.

laanwj · 2017-08-25T13:14:24Z

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 · 2017-08-25T13:19:43Z

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

laanwj · 2017-08-25T13:24:54Z

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 · 2017-08-25T13:28:08Z

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.

promag · 2017-08-25T13:32:37Z

src/rpc/protocol.cpp

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.

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

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.

Let's ask @theuni just in case.

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 · 2017-08-25T13:33:50Z

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...

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 bitcoin#11129.

gmaxwell

Concept ACK

meshcollider · 2017-08-26T12:40:54Z

utACK 82dd719

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

Agreed, this seems fine to me

sipa · 2017-08-27T18:54:33Z

utACK 82dd719

82dd719 rpc: Write authcookie atomically (Wladimir J. van der Laan) Pull request description: 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. Tree-SHA512: 47fcc1ed2ff3d8fed4b7441e4939f29cc99b57b7a035673c3b55a124a2e49c8a904637a6ff700dd13a184be8c0255707d74781f8e626314916418954e2467e03

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 bitcoin#11129. Github-Pull: bitcoin#11131 Rebased-From: 82dd719

Summary: 82dd719 rpc: Write authcookie atomically (Wladimir J. van der Laan) Pull request description: 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. Tree-SHA512: 47fcc1ed2ff3d8fed4b7441e4939f29cc99b57b7a035673c3b55a124a2e49c8a904637a6ff700dd13a184be8c0255707d74781f8e626314916418954e2467e03 Backport of Core PR11131 bitcoin/bitcoin#11131 Test Plan: make check ./bitcoind Verify `.cookie` exists ine `.bitcoin/<dir>/.cookie` Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3940

82dd719 rpc: Write authcookie atomically (Wladimir J. van der Laan) Pull request description: 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 bitcoin#11129. As such, this is an alternative to that PR. Tree-SHA512: 47fcc1ed2ff3d8fed4b7441e4939f29cc99b57b7a035673c3b55a124a2e49c8a904637a6ff700dd13a184be8c0255707d74781f8e626314916418954e2467e03

Summary: 82dd719 rpc: Write authcookie atomically (Wladimir J. van der Laan) Pull request description: 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. Tree-SHA512: 47fcc1ed2ff3d8fed4b7441e4939f29cc99b57b7a035673c3b55a124a2e49c8a904637a6ff700dd13a184be8c0255707d74781f8e626314916418954e2467e03 Backport of Core PR11131 bitcoin/bitcoin#11131 Test Plan: make check ./bitcoind Verify `.cookie` exists ine `.bitcoin/<dir>/.cookie` Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3940

laanwj added the RPC/REST/ZMQ label Aug 25, 2017

laanwj mentioned this pull request Aug 25, 2017

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

Closed

promag reviewed Aug 25, 2017

View reviewed changes

laanwj force-pushed the 2017_08_atomic_cookie branch from 43e7dcb to e413f1d Compare August 25, 2017 13:31

promag reviewed Aug 25, 2017

View reviewed changes

rpc: Write authcookie atomically

82dd719

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 bitcoin#11129.

laanwj force-pushed the 2017_08_atomic_cookie branch from e413f1d to 82dd719 Compare August 25, 2017 13:38

maflcko added this to the 0.15.1 milestone Aug 25, 2017

maflcko added the Needs backport label Aug 25, 2017

gmaxwell reviewed Aug 25, 2017

View reviewed changes

maflcko requested a review from theuni August 27, 2017 13:34

laanwj merged commit 82dd719 into bitcoin:master Aug 28, 2017

maflcko removed the Needs backport label Oct 4, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

rpc: Write authcookie atomically #11131

rpc: Write authcookie atomically #11131

Uh oh!

Conversation

laanwj commented Aug 25, 2017

Uh oh!

maflcko commented Aug 25, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Aug 25, 2017

Uh oh!

laanwj commented Aug 25, 2017

Uh oh!

promag commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 25, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Aug 26, 2017

Uh oh!

sipa commented Aug 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maflcko commented Aug 25, 2017 via email •

edited

Loading

promag left a comment •

edited

Loading

laanwj Aug 25, 2017 •

edited

Loading

laanwj commented Aug 25, 2017 •

edited

Loading