rpc: keep `.cookie` file if it was not generated

romanz commented at 3:30 pm on November 3, 2023: contributor

Otherwise, starting bitcoind twice may cause the .cookie file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock).

rpc: keep .cookie if it was not generated

Otherwise, starting bitcoind twice may cause the `.cookie`
file generated by the first instance to be deleted by the
second instance shutdown (after failing to obtain a lock).

7cb9367157

DrahtBot commented at 3:30 pm on November 3, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	kristapsk, stickies-v, willcl-ark, achow101
Stale ACK	luke-jr, maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28946 (init: don’t delete PID file if it was not generated by willcl-ark)
#28167 (init: Add option for rpccookie permissions (replace 26088) by willcl-ark)

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.

DrahtBot added the label RPC/REST/ZMQ on Nov 3, 2023

romanz commented at 3:41 pm on November 3, 2023: contributor

The bug can be reproduced manually by starting bitcoind twice, resulting in a missing .cookie file:

 0$ bitcoind -signet &
 1...
 22023-11-03T15:38:09Z Using random cookie authentication.
 32023-11-03T15:38:09Z Generated RPC authentication cookie /home/user/.bitcoin/signet/.cookie
 4...
 5
 6$ ls ~/.bitcoin/signet/.cookie 
 7/home/user/.bitcoin/signet/.cookie
 8
 9$ bitcoind -signet
10Error: Cannot obtain a lock on data directory /home/user/.bitcoin/signet. Bitcoin Core is probably already running.
11
12$ ls /home/user/.bitcoin/signet/.cookie 
13ls: cannot access '/home/user/.bitcoin/signet/.cookie': No such file or directory

romanz renamed this:
~~rpc: keep .cookie if it was not generated~~
rpc: keep `.cookie` file if it was not generated
on Nov 3, 2023

luke-jr approved

luke-jr commented at 3:40 pm on November 8, 2023: member

utACK, though it might be better to do something like keep the file open and detect if it’s the same file/content before we delete it.

kristapsk approved

kristapsk commented at 4:00 pm on November 8, 2023: contributor

utACK 7cb9367157eb42ee06bc6fa024522cc14a80138d

DrahtBot requested review from luke-jr on Nov 8, 2023

luke-jr approved

luke-jr commented at 9:34 pm on November 8, 2023: member

utACK after squash

luke-jr referenced this in commit 0d1452ace8 on Nov 9, 2023

luke-jr referenced this in commit 80cb773848 on Nov 9, 2023

romanz force-pushed on Nov 9, 2023

romanz commented at 3:50 am on November 9, 2023: contributor

Thanks for the review! Squashed and force-pushed d95dde9441fb791046394ed3784a840a54ef2ab9.

DrahtBot added the label CI failed on Nov 9, 2023

DrahtBot removed the label CI failed on Nov 9, 2023

kristapsk approved

kristapsk commented at 5:48 am on November 9, 2023: contributor

re-ACK d95dde9441fb791046394ed3784a840a54ef2ab9

DrahtBot requested review from luke-jr on Nov 9, 2023

luke-jr commented at 10:57 pm on November 9, 2023: member

re-ACK d95dde9441fb791046394ed3784a840a54ef2ab9

DrahtBot removed review request from luke-jr on Nov 9, 2023

luke-jr approved

luke-jr referenced this in commit 4ec79e3375 on Nov 10, 2023

romanz commented at 6:44 pm on November 16, 2023: contributor

Just wanted to ask whether there are any other outstanding issues? :)

maflcko added this to the milestone 27.0 on Nov 17, 2023

maflcko removed this from the milestone 27.0 on Nov 17, 2023

romanz commented at 7:15 pm on November 24, 2023: contributor

Ping :)

willcl-ark approved

willcl-ark commented at 10:28 am on November 27, 2023: contributor

ACK d95dde9441fb791046394ed3784a840a54ef2ab9

Kind of suprised this wasn’t noticed earlier, but thanks for the patch!

I also noticed while testing this that the same issue is present with the pid file, but I will open a seperate pull for that in a moment.

maflcko commented at 10:35 am on November 27, 2023: member

lgtm ACK d95dde9441fb791046394ed3784a840a54ef2ab9

in src/rpc/request.cpp:138 in d95dde9441 outdated

133@@ -131,7 +134,11 @@ bool GetAuthCookie(std::string *cookie_out)
134 void DeleteAuthCookie()
135 {
136     try {
137-        fs::remove(GetAuthCookieFile());
138+        std::string existing_cookie;
139+        if (GetAuthCookie(&existing_cookie) && g_generated_cookie == existing_cookie) {

stickies-v commented at 3:08 pm on November 27, 2023:

If g_generated_cookie is not nullopt, will g_generated_cookie == existing_cookie ever be false? I don’t think so, and I think that makes this check a bit misleading. I would thus either convert g_generated_cookie into a bool (as e.g. motivated here), or as suggested by myself in #28946 (review) use a std::optional<fs::path> but then perform this check only on the truthiness of g_generated_cookie, and not its value.

 0diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
 1index c706ddf8d5..937a3a7dd3 100644
 2--- a/src/rpc/request.cpp
 3+++ b/src/rpc/request.cpp
 4@@ -80,7 +80,7 @@ static fs::path GetAuthCookieFile(bool temp=false)
 5     return AbsPathForConfigVal(gArgs, arg);
 6 }
 7 
 8-static std::optional<std::string> g_generated_cookie;
 9+static std::optional<fs::path> g_generated_cookie;
10 
11 bool GenerateAuthCookie(std::string *cookie_out)
12 {
13@@ -107,7 +107,7 @@ bool GenerateAuthCookie(std::string *cookie_out)
14         LogPrintf("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath));
15         return false;
16     }
17-    g_generated_cookie = cookie;
18+    g_generated_cookie = filepath;
19     LogPrintf("Generated RPC authentication cookie %s\n", fs::PathToString(filepath));
20 
21     if (cookie_out)
22@@ -133,12 +133,10 @@ bool GetAuthCookie(std::string *cookie_out)
23 
24 void DeleteAuthCookie()
25 {
26+    // Delete the cookie file if it exists and was generated by this process
27+    if (!g_generated_cookie) return;
28     try {
29-        std::string existing_cookie;
30-        if (GetAuthCookie(&existing_cookie) && g_generated_cookie == existing_cookie) {
31-            // Delete the cookie file if it exists and was generated by this process
32-            fs::remove(GetAuthCookieFile());
33-        }
34+            fs::remove(g_generated_cookie.value());
35     } catch (const fs::filesystem_error& e) {
36         LogPrintf("%s: Unable to remove random auth cookie file: %s\n", __func__, fsbridge::get_filesystem_error_message(e));
37     }

stickies-v commented at 3:17 pm on November 27, 2023:

Or combining this with a fs::remove simplification:

 0diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
 1index c706ddf8d5..06962d2b87 100644
 2--- a/src/rpc/request.cpp
 3+++ b/src/rpc/request.cpp
 4@@ -80,7 +80,7 @@ static fs::path GetAuthCookieFile(bool temp=false)
 5     return AbsPathForConfigVal(gArgs, arg);
 6 }
 7 
 8-static std::optional<std::string> g_generated_cookie;
 9+static std::optional<fs::path> g_generated_cookie;
10 
11 bool GenerateAuthCookie(std::string *cookie_out)
12 {
13@@ -107,7 +107,7 @@ bool GenerateAuthCookie(std::string *cookie_out)
14         LogPrintf("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath));
15         return false;
16     }
17-    g_generated_cookie = cookie;
18+    g_generated_cookie = filepath;
19     LogPrintf("Generated RPC authentication cookie %s\n", fs::PathToString(filepath));
20 
21     if (cookie_out)
22@@ -133,14 +133,10 @@ bool GetAuthCookie(std::string *cookie_out)
23 
24 void DeleteAuthCookie()
25 {
26-    try {
27-        std::string existing_cookie;
28-        if (GetAuthCookie(&existing_cookie) && g_generated_cookie == existing_cookie) {
29-            // Delete the cookie file if it exists and was generated by this process
30-            fs::remove(GetAuthCookieFile());
31-        }
32-    } catch (const fs::filesystem_error& e) {
33-        LogPrintf("%s: Unable to remove random auth cookie file: %s\n", __func__, fsbridge::get_filesystem_error_message(e));
34+    // Delete the cookie file if it exists and was generated by this process
35+    if (!g_generated_cookie) return;
36+    if (std::error_code error; !fs::remove(g_generated_cookie.value(), error) && error) {
37+        LogPrintf("%s: Unable to remove random auth cookie file: %s\n", error.message());
38     }
39 }
40

romanz commented at 6:57 pm on November 27, 2023:

Initially I used a boolean flag (https://github.com/bitcoin/bitcoin/commit/7cb9367157eb42ee06bc6fa024522cc14a80138d) but following this comment it was changed to contain the cookie contents.

I agree that g_generated_cookie == existing_cookie can be false only if the cookie was somehow overwritten by some other process (i.e. not by bitcoind), so indeed it would be simpler to use the patches suggested above. @luke-jr what do you think?

romanz commented at 7:31 pm on November 27, 2023:

@willcl-ark I also prefer to be consistent with the solution chosen for #28946, so please let me know which fix is preferred.

achow101 commented at 10:33 pm on November 27, 2023:

I think it would be better to stick to using a boolean flag since checking the contents currently is not meaningfully useful or different.

romanz commented at 7:26 pm on November 30, 2023:

Sounds good, reverted to use a boolean flag.

romanz force-pushed on Nov 30, 2023

maflcko commented at 7:34 pm on November 30, 2023: member

Note: If you revert to 7cb9367157eb42ee06bc6fa024522cc14a80138d instead (which is the same), the previous reviews will be valid again

romanz force-pushed on Nov 30, 2023

romanz commented at 7:46 pm on November 30, 2023: contributor

Note: If you revert to 7cb9367 instead (which is the same), the previous reviews will be valid again

Thanks - reverted to https://github.com/bitcoin/bitcoin/commit/7cb9367157eb42ee06bc6fa024522cc14a80138d.

kristapsk approved

kristapsk commented at 8:41 pm on November 30, 2023: contributor

re-ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d

DrahtBot requested review from luke-jr on Nov 30, 2023

DrahtBot requested review from willcl-ark on Nov 30, 2023

DrahtBot requested review from maflcko on Nov 30, 2023

in src/rpc/request.cpp:137 in 7cb9367157

133@@ -131,7 +134,10 @@ bool GetAuthCookie(std::string *cookie_out)
134 void DeleteAuthCookie()
135 {
136     try {
137-        fs::remove(GetAuthCookieFile());
138+        if (g_generated_cookie) {

stickies-v commented at 11:39 pm on November 30, 2023:

nit: can be done without extra nesting

 0diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
 1index b7acd62ee3..ad9f244f12 100644
 2--- a/src/rpc/request.cpp
 3+++ b/src/rpc/request.cpp
 4@@ -133,11 +133,10 @@ bool GetAuthCookie(std::string *cookie_out)
 5 
 6 void DeleteAuthCookie()
 7 {
 8+    // Don't delete the cookie file if it wasn't generated by this process
 9+    if (!g_generated_cookie) return;
10     try {
11-        if (g_generated_cookie) {
12-            // Delete the cookie file if it was generated by this process
13-            fs::remove(GetAuthCookieFile());
14-        }
15+        fs::remove(GetAuthCookieFile());
16     } catch (const fs::filesystem_error& e) {
17         LogPrintf("%s: Unable to remove random auth cookie file: %s\n", __func__, fsbridge::get_filesystem_error_message(e));
18     }

stickies-v approved

stickies-v commented at 11:39 pm on November 30, 2023: contributor

ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d

willcl-ark commented at 11:29 am on December 1, 2023: contributor

re-ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d

DrahtBot removed review request from willcl-ark on Dec 1, 2023

achow101 commented at 5:17 pm on December 1, 2023: member

ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d

achow101 merged this on Dec 1, 2023

achow101 closed this on Dec 1, 2023

romanz deleted the branch on Dec 1, 2023

romanz commented at 6:33 pm on December 1, 2023: contributor

Thanks :)

achow101 referenced this in commit afdc4c3a30 on Dec 4, 2023

luke-jr referenced this in commit cd02d38941 on Jan 26, 2024

luke-jr referenced this in commit 33c0e8a03d on Jan 26, 2024

achow101 added the label Needs backport (26.x) on Feb 22, 2024

achow101 added this to the milestone 26.1 on Feb 22, 2024

darosior referenced this in commit 1e956439eb on Feb 28, 2024

glozow referenced this in commit a718bfafe7 on Feb 28, 2024

glozow removed the label Needs backport (26.x) on Feb 28, 2024

glozow commented at 5:07 pm on March 6, 2024: member

this was backported for 26.x in #29503

rpc: keep .cookie file if it was not generated #28784

Code Coverage

Reviews

Conflicts

rpc: keep `.cookie` file if it was not generated #28784