Fix locking on WSL using flock instead of fcntl #18700

meshcollider · 2020-04-19T00:35:27Z

A bug in WSL means that fcntl does not exclusively lock files, allowing multiple instances of bitcoin to use the same datadir. If we instead use flock, it works correctly. Passes Travis, but testing on some OS variety would be sensible.

From what I can tell, flock and fcntl don't work with each other on linux, so it would still be possible to run a node with this code change and a node before it with the same datadir (this isn't true for Mac/FreeBSD). flock also doesn't support NFS on MacOS and linux<2.6.12 while fcntl did. See here for example: https://gavv.github.io/articles/file-locks/

If changing to flock for all systems is inadvisable, it would also be possible to just detect WSL and use flock when on that platform to avoid the bug.

DrahtBot · 2020-04-20T04:43:16Z

Gitian builds

File	commit `6ae99aa` (master)	commit `1a36387` (master and this pull)
bitcoin-0.20.99-aarch64-linux-gnu-debug.tar.gz	`9e74baea5087f75d...`	`dd9caf6fa1126a3f...`
bitcoin-0.20.99-aarch64-linux-gnu.tar.gz	`b97f8fed4ef242bc...`	`5a3144105bfe97ca...`
bitcoin-0.20.99-arm-linux-gnueabihf-debug.tar.gz	`c3cafca0d2c385b7...`	`16c59265a3f49440...`
bitcoin-0.20.99-arm-linux-gnueabihf.tar.gz	`9cf7842f6c7e72f9...`	`d3117c583c9ae443...`
bitcoin-0.20.99-osx-unsigned.dmg	`b8882f2b1e257bee...`	`279210aeb73a73ea...`
bitcoin-0.20.99-osx64.tar.gz	`9100c2ace54ffeb2...`	`fc5834c15be4b962...`
bitcoin-0.20.99-riscv64-linux-gnu-debug.tar.gz	`e3794553bdeb3406...`	`2f6bad18dccf3b6f...`
bitcoin-0.20.99-riscv64-linux-gnu.tar.gz	`3d60c7b6ad3a3b7a...`	`05d470ffe6c8efac...`
bitcoin-0.20.99-win64-debug.zip	`923a2a8d708b1196...`	`33b8e7fbaabee227...`
bitcoin-0.20.99-win64-setup-unsigned.exe	`4fd60fb4ee6d252a...`	`e32c5565b0039492...`
bitcoin-0.20.99-win64.zip	`014a0e245da57de1...`	`f74643de10c60c9c...`
bitcoin-0.20.99-x86_64-linux-gnu-debug.tar.gz	`6b72060bfdf9cd8a...`	`21ddfffa1ee0f8e9...`
bitcoin-0.20.99-x86_64-linux-gnu.tar.gz	`d2e0d4726ecd84f6...`	`9eec3e7ef9c754bc...`
bitcoin-0.20.99.tar.gz	`5f8eaab85a59877b...`	`fe3dd2ab68a6220f...`
bitcoin-core-linux-0.21-res.yml	`b9c0b8516f1f7476...`	`799ccd04f4502d7c...`
bitcoin-core-osx-0.21-res.yml	`aa56d3073ba6853c...`	`fa86f0cc5bfb2217...`
bitcoin-core-win-0.21-res.yml	`127d7553b4c8553c...`	`938bc5b198d8f409...`
linux-build.log	`826d7ba2297e1251...`	`5d1e9e9b6410d255...`
osx-build.log	`a889d7e96df8cc6a...`	`4023f21916323bf7...`
win-build.log	`6311c7e3c77d0b67...`	`37762874f39f9ee8...`
bitcoin-core-linux-0.21-res.yml.diff		`eeb2e55b504104ca...`
bitcoin-core-osx-0.21-res.yml.diff		`c465a6caf5f4f7d5...`
bitcoin-core-win-0.21-res.yml.diff		`6e7e44435c9ef0a9...`
linux-build.log.diff		`b6ed5e37226cd11c...`
osx-build.log.diff		`67b8c8e6f6898f29...`
win-build.log.diff		`c39e17d5668588b9...`

laanwj · 2020-04-22T11:07:20Z

If this is a bug in WSL, why not wait for them to fix it instead? I don't think it's desirable to change our handling of locks on all UNIX platforms just for a bug in their implementation. If we need is, please add it for WSL only and add a comment that it can be removed again when the upstream bug is fixed (with a link to the upstream bug).

laanwj · 2020-04-22T11:15:53Z

Can you please test that this fixes your issue @mtrycz.

meshcollider · 2020-04-22T12:24:14Z

@laanwj the bug was reported upstream in 2017 with no fix in sight, so it seems it is up to us

mtrycz · 2020-04-22T13:27:23Z

I've ran my test plan (from the related issue), and I can confirm that the test fails in the latest master 9e8e813d and the v0.19.1 tag, and that it passes in this Pull Request.

However, I'm running more extended tests and some are failing. Please allow some time to make an adequate description.

wumpus · 2020-04-22T15:30:44Z

Github @wumpus is not bitcoin wumpus.

sipa · 2020-04-23T02:59:12Z

src/fs.cpp

+    // Exclusive file locking is broken on WSL using fcntl (issue #18622)
+    // This workaround can be removed once the bug on WSL is fixed
+    struct utsname uname_data;
+    if (uname(&uname_data) == 0 && std::string(uname_data.version).find("Microsoft") != std::string::npos) {


This condition should probably be cached in a static bool local variable.

EDIT: and only conditionally compiled for WIN32?

@sipa it isn't "win32" because its the linux subsystem, so it runs in the non-windows branch

Still, please cache the result.

mtrycz · 2020-04-23T07:30:28Z

I have ran test/functional/test_runner.py several times both on master and this PR.

The specific issue is solved for WSL on this PR's code.

There are also several test failures (on both branches) that are reproducible, and some that are non-deterministic. If this sounds new, should I open a new issue with this?

sipa · 2020-04-24T01:20:47Z

src/fs.cpp

+    // Exclusive file locking is broken on WSL using fcntl (issue #18622)
+    // This workaround can be removed once the bug on WSL is fixed
+    // Cache the result to avoid multiple system calls
+    static bool is_wsl_cache_exists = false;


It's a crazy edge case, and unlikely on realistic platforms, but this is technically UB if two threads simultaneously reach the if (!is_wsl_cache_exists) check. The most C++ish solution is this I think: sipa@9470c59.

luke-jr · 2020-04-24T21:13:04Z

Is there any reason we can't check/take both locks?

Co-authored-by: sipa <pieter@wuille.net>

meshcollider · 2020-04-26T00:17:28Z

Thanks @sipa, updated :)

laanwj · 2020-05-11T12:13:47Z

I'm removing this from the 0.20.0 milestone. It's not a regression in 0.20, it's an upstream bug in "Linux emulation", ~~and I think this fix is not quite ready yet.~~

laanwj · 2020-05-28T15:18:56Z

Code review ACK e8fa0a3

laanwj · 2020-05-28T15:57:54Z

src/fs.cpp

@@ -47,20 +50,38 @@ FileLock::~FileLock()
    }
 }

+static bool IsWSL()


I just realized after merging that we could shortcut this check to false on non-x86 platforms. But it doesn't matter too much.

maflcko · 2020-05-28T16:17:04Z

Tagged with " Needs backport (0.20) "

e8fa0a3 Fix WSL file locking by using flock instead of fcntl (Samuel Dobson) Pull request description: Fixes bitcoin#18622 A bug in WSL means that fcntl does not exclusively lock files, allowing multiple instances of bitcoin to use the same datadir. If we instead use flock, it works correctly. Passes Travis, but testing on some OS variety would be sensible. From what I can tell, flock and fcntl don't work with each other on linux, so it would still be possible to run a node with this code change and a node before it with the same datadir (this isn't true for Mac/FreeBSD). flock also doesn't support NFS on MacOS and linux<2.6.12 while fcntl did. See here for example: https://gavv.github.io/articles/file-locks/ If changing to flock for all systems is inadvisable, it would also be possible to just detect WSL and use flock when on that platform to avoid the bug. ACKs for top commit: laanwj: Code review ACK e8fa0a3 Tree-SHA512: ca1009e171970101f1dc2332c5e998717aee00eebc80bb586b826927a74bd0d4c94712e46d1396821bc30533d76deac391b6e1c406c406865661f57fa062c702

Summary: Co-authored-by: sipa <pieter@wuille.net> Backport of Core [[bitcoin/bitcoin#18700 | PR18700]] Test Plan: On WSL ninja check ninja check-functional Confirm all tests pass except `feature_proxy.py` (fails), `rpc_bind.py` (skipped), and `interface_zmq.py` (skipped). Using two different consoles: src/bitcoind Verify this fails on the second console with the following error: Error: Cannot obtain a lock on data directory <path>/.bitcoin. Bitcoin ABC is probably already running. Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6299

Co-authored-by: sipa <pieter@wuille.net> Github-Pull: bitcoin#18700 Rebased-From: e8fa0a3

e8fa0a3 Fix WSL file locking by using flock instead of fcntl (Samuel Dobson) Pull request description: Fixes bitcoin#18622 A bug in WSL means that fcntl does not exclusively lock files, allowing multiple instances of bitcoin to use the same datadir. If we instead use flock, it works correctly. Passes Travis, but testing on some OS variety would be sensible. From what I can tell, flock and fcntl don't work with each other on linux, so it would still be possible to run a node with this code change and a node before it with the same datadir (this isn't true for Mac/FreeBSD). flock also doesn't support NFS on MacOS and linux<2.6.12 while fcntl did. See here for example: https://gavv.github.io/articles/file-locks/ If changing to flock for all systems is inadvisable, it would also be possible to just detect WSL and use flock when on that platform to avoid the bug. ACKs for top commit: laanwj: Code review ACK e8fa0a3 Tree-SHA512: ca1009e171970101f1dc2332c5e998717aee00eebc80bb586b826927a74bd0d4c94712e46d1396821bc30533d76deac391b6e1c406c406865661f57fa062c702

cculianu · 2020-06-18T11:10:20Z

Note that this patch only answers the question IsWSL? for WSL ver 1. Ver 2 returns a totally different uts_name.version string as such:

WSL2 uts_name.version string: #1 SMP Wed Feb 19 06:37:35 UTC 2020

$ uname -a
Linux DESKTOP-TPEL8HN 4.19.104-microsoft-standard #1 SMP Wed Feb 19 06:37:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

I am running on Win10 WSL2 with Ubuntu 20.04. See the attached C++ test program to convince yourself.

#include <iostream>
#include <string>

#include <sys/file.h>
#include <sys/utsname.h>

static bool IsWSL(std::string *out = nullptr) {
	struct utsname uname_data;
	auto res = uname(&uname_data);
	std::string ver{uname_data.version};
	bool ret = res == 0 && ver.find("Microsoft") != std::string::npos;
	if (out) *out = std::move(ver);
	return ret;
}

int main()
{
	using std::cout;

	std::string ver;
	auto b = IsWSL(&ver);
	cout << "Ver: " << ver << '\n';
	cout << "IsWSL?: " << b << '\n';
	return 0;
}

I don't know if the bug #18622 is still an issue on WSL2. I'm going to try and determine this and write back here when I do.

cculianu · 2020-06-18T11:38:32Z

Follow-up: Can confirm that the upstream bug from microsoft on fcntl F_SETLK is fixed on WSL2. The linux/unix execution path that is taken on WSL2 (because IsWSL() returns false here) leads to correct behavior and the lock succeeds.

Suggest: Rename this function to IsWSL1 to make it clearer when it will return true and/or add a comment to that effect...

2b79ac7 Clean up separated ban/discourage interface (Pieter Wuille) 0477348 Replace automatic bans with discouragement filter (Pieter Wuille) e7f06f9 test: remove Cirrus CI FreeBSD job (fanquake) eb6b82a qa: Test concurrent wallet loading (João Barbosa) c9b49d2 wallet: Handle concurrent wallet loading (João Barbosa) cf0b5a9 tests: Check that segwit inputs in psbt have both UTXO types (Andrew Chow) 3228b59 psbt: always put a non_witness_utxo and don't remove it (Andrew Chow) ed5ec30 psbt: Allow both non_witness_utxo and witness_utxo (Andrew Chow) 68e0e6f rpc: show both UTXOs in decodepsbt (Andrew Chow) 27786d0 trivial: Suggested cleanups to surrounding code (Russell Yanofsky) 654420d wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky) febebc4 Fix WSL file locking by using flock instead of fcntl (Samuel Dobson) 5c7151a gui: update Qt base translations for macOS release (fanquake) c219d21 build: improved output of configure for build OS (sachinkm77) 0596a6e util: Don't reference errno when pthread fails. (MIZUTA Takeshi) Pull request description: Currently backports the following to the 0.20 branch: * #18700 - Fix locking on WSL using flock instead of fcntl * #18982 - wallet: Minimal fix to restore conflicted transaction notifications * #19059 - gui: update Qt base translations for macOS release * #19152 - build: improve build OS configure output * #19194 - util: Don't reference errno when pthread fails. * #19215 - psbt: Include and allow both non_witness_utxo and witness_utxo for segwit inputs * #19219 - Replace automatic bans with discouragement filter * #19300 - wallet: Handle concurrent wallet loading ACKs for top commit: amitiuttarwar: ACK 0477348 2b79ac7 by comparing to original changes, double checking the diff sipa: utACK 2b79ac7 laanwj: ACK 2b79ac7 Tree-SHA512: e5eb31d08288fa4a6e8aba08e60b16ce1988f14f249238b1cfd18ab2c8f6fcd9f07e3c0c491d32d2361fca26e3037fb0374f37464bddcabecea29069d6737539

Summary: Co-authored-by: sipa <pieter@wuille.net> Backport of Core [[bitcoin/bitcoin#18700 | PR18700]] Test Plan: On WSL ninja check ninja check-functional Confirm all tests pass except `feature_proxy.py` (fails), `rpc_bind.py` (skipped), and `interface_zmq.py` (skipped). Using two different consoles: src/bitcoind Verify this fails on the second console with the following error: Error: Cannot obtain a lock on data directory <path>/.bitcoin. Bitcoin ABC is probably already running. Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6299

Co-authored-by: sipa <pieter@wuille.net> Github-Pull: #18700 Rebased-From: e8fa0a3

Co-authored-by: sipa <pieter@wuille.net> Github-Pull: bitcoin#18700 Rebased-From: e8fa0a3

e8fa0a3 Fix WSL file locking by using flock instead of fcntl (Samuel Dobson) Pull request description: Fixes bitcoin#18622 A bug in WSL means that fcntl does not exclusively lock files, allowing multiple instances of bitcoin to use the same datadir. If we instead use flock, it works correctly. Passes Travis, but testing on some OS variety would be sensible. From what I can tell, flock and fcntl don't work with each other on linux, so it would still be possible to run a node with this code change and a node before it with the same datadir (this isn't true for Mac/FreeBSD). flock also doesn't support NFS on MacOS and linux<2.6.12 while fcntl did. See here for example: https://gavv.github.io/articles/file-locks/ If changing to flock for all systems is inadvisable, it would also be possible to just detect WSL and use flock when on that platform to avoid the bug. ACKs for top commit: laanwj: Code review ACK e8fa0a3 Tree-SHA512: ca1009e171970101f1dc2332c5e998717aee00eebc80bb586b826927a74bd0d4c94712e46d1396821bc30533d76deac391b6e1c406c406865661f57fa062c702

8c4b365 Fix WSL file locking by using flock instead of fcntl (Samuel Dobson) 189de2f Avoid redefine warning (Peter Bushnell) Pull request description: Straight forward backport of bitcoin#15782 and bitcoin#18700 to address a bug in WSL1 environments that results in improper locking behavior; ie, a directory lock is not made exclusive as intended, thus resulting in multiple instances of the wallet/daemon being able to access the same datadir simultaneously instead of erroring out due to a locking conflict as intended. This is specific to WSL1 environments, as WSL2 (not yet fully supported/documented), standard linux, and macOS environments behave as intended. ACKs for top commit: furszy: good, utACK 8c4b365 random-zebra: utACK 8c4b365 and merging... Tree-SHA512: 7ba7d054a858baae4df2ae6daa4d3ffc694bcff0b48958ba28b3203d6dc0d950d25596eb324dc3973b09a65a251946c29be035959b3da7ecf126f9110b7056d0

fanquake added the Windows label Apr 19, 2020

meshcollider added this to the 0.20.0 milestone Apr 19, 2020

fanquake added the Utils/log/libs label Apr 19, 2020

maflcko added the Needs gitian build label Apr 19, 2020

DrahtBot removed the Needs gitian build label Apr 20, 2020

sipa reviewed Apr 23, 2020

View reviewed changes

sipa reviewed Apr 24, 2020

View reviewed changes

Fix WSL file locking by using flock instead of fcntl

e8fa0a3

Co-authored-by: sipa <pieter@wuille.net>

maflcko mentioned this pull request May 8, 2020

util_tests/test_LockDirectory failure running tests in WSL guest #14219

Closed

laanwj modified the milestones: 0.20.0, 0.20.1 May 11, 2020

laanwj merged commit ea3e9e0 into bitcoin:master May 28, 2020

laanwj reviewed May 28, 2020

View reviewed changes

maflcko added the Needs backport (0.20) label May 28, 2020

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 9, 2020

Fix WSL file locking by using flock instead of fcntl

febebc4

Co-authored-by: sipa <pieter@wuille.net> Github-Pull: bitcoin#18700 Rebased-From: e8fa0a3

fanquake mentioned this pull request Jun 9, 2020

[0.20] Backports #19224

Merged

fanquake removed the Needs backport (0.20) label Jun 9, 2020

backpacker69 referenced this pull request in peercoin/peercoin Sep 8, 2020

Fix WSL file locking by using flock instead of fcntl

c98452e

Co-authored-by: sipa <pieter@wuille.net> Github-Pull: #18700 Rebased-From: e8fa0a3

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Oct 21, 2020

Fix WSL file locking by using flock instead of fcntl

cb55273

Co-authored-by: sipa <pieter@wuille.net> Github-Pull: bitcoin#18700 Rebased-From: e8fa0a3

Platinumwrist referenced this pull request in syscoin/syscoin Oct 25, 2020

doc: Add historical release notes for 0.20.1

05e2740

yumeyao mentioned this pull request Jan 20, 2021

fcntl(F_SETLK) doestn' work yumeyao/WSL-quickfix#2

Open

Fuzzbawls mentioned this pull request Aug 8, 2021

[Bug] Fix improper locking on Windows WSL1 environments PIVX-Project/PIVX#2497

Merged

barton2526 mentioned this pull request Aug 31, 2021

Fix locking on WSL using flock instead of fcntl gridcoin-community/Gridcoin-Research#2314

Merged

str4d mentioned this pull request Dec 6, 2021

Backport changes to src/fs.{cpp,h} zcash/zcash#5406

Open

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Fix locking on WSL using flock instead of fcntl #18700

Fix locking on WSL using flock instead of fcntl #18700

Uh oh!

Conversation

meshcollider commented Apr 19, 2020

Uh oh!

DrahtBot commented Apr 20, 2020

Gitian builds

Uh oh!

laanwj commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Apr 22, 2020

Uh oh!

meshcollider commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrycz commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wumpus commented Apr 22, 2020

Uh oh!

sipa Apr 23, 2020 • 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!

meshcollider Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

sipa Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

sipa Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

mtrycz commented Apr 23, 2020

Uh oh!

sipa Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Apr 24, 2020

Uh oh!

meshcollider commented Apr 26, 2020

Uh oh!

laanwj commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented May 28, 2020

Uh oh!

laanwj May 28, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 28, 2020

Uh oh!

cculianu commented Jun 18, 2020

Uh oh!

cculianu commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

laanwj commented Apr 22, 2020 •

edited

Loading

meshcollider commented Apr 22, 2020 •

edited

Loading

mtrycz commented Apr 22, 2020 •

edited

Loading

sipa Apr 23, 2020 •

edited

Loading

laanwj commented May 11, 2020 •

edited

Loading

cculianu commented Jun 18, 2020 •

edited

Loading