wallet: prevent the same wallet file from being opened by multiple processes #3994

stoffu · 2018-06-12T06:33:26Z

Maybe this way of trying to lock a file is stupid, but perhaps better than nothing...

moneromooo-monero · 2018-06-20T13:47:47Z

I had an idea of doing this via advisory locks, which are cleared when a process ends. I'm not sure it can be done cross platform though. If the wallet crashes or gets killed, this lock file doesn't go away. Maybe that's good enough though.

hyc · 2018-06-20T14:47:07Z

Advisory locks would certainly be cleaner. We had locking code for the blockchain in monerod before, can just troll thru git history and paste that in here instead.

stoffu · 2018-06-21T13:42:32Z

@moneromooo-monero @hyc
Thanks a lot for your suggestion, I've updated the patch accordingly.

I'm quite unfamiliar with Windows/Linux APIs so I might be using them inappropriately, but it seems to be working as far as I tested with Mac, Ubuntu and Windows.

stoffu · 2018-06-21T13:47:14Z

src/wallet/wallet2.cpp


+  m_keys_file_locker.reset();


I realized this temporary unlocking was necessary due to Windows (otherwise the file couldn't be loaded).

Maybe add a comment saying so ?

moneromooo-monero · 2018-06-23T12:57:25Z

src/common/util.cpp

+  {
+#ifdef WIN32
+    m_fd = INVALID_HANDLE_VALUE;
+    WCHAR wide_path[1000];


Is there no PATH_MAX for Windows ?

This was copied from code added in #3313. It seems like Windows also has _PATH_MAX, but the value might change depending on different versions of Windows, and it doesn't seem like the universal way of guaranteeing sufficient length for path names. The current codebase has two places where MultiByteToWideChar is used:

monero/external/db_drivers/liblmdb/mdb.c

Lines 10942 to 10959 in ffab670

static int utf8_to_utf16(const char *src, int srcsize, wchar_t **dst, int *dstsize)

{

int need;

wchar_t *result;

need = MultiByteToWideChar(CP_UTF8, 0, src, srcsize, NULL, 0);

if (need == 0xFFFD)

return EILSEQ;

if (need == 0)

return EINVAL;

result = malloc(sizeof(wchar_t) * need);

if (!result)

return ENOMEM;

MultiByteToWideChar(CP_UTF8, 0, src, srcsize, result, need);

if (dstsize)

*dstsize = need;

*dst = result;

return 0;

}

monero/tests/gtest/src/gtest.cc

Lines 858 to 869 in ffab670

LPCWSTR String::AnsiToUtf16(const char* ansi) {

if (!ansi) return NULL;

const int length = strlen(ansi);

const int unicode_length =

MultiByteToWideChar(CP_ACP, 0, ansi, length,

NULL, 0);

WCHAR* unicode = new WCHAR[unicode_length + 1];

MultiByteToWideChar(CP_ACP, 0, ansi, length,

unicode, unicode_length);

unicode[unicode_length] = 0;

return unicode;

}

In both cases, the length of the resulting UTF-16 string is obtained by first calling the function with the output location set to NULL, which seems like the right way.

If this way is to be used, I think it makes sense to also replace the corresponding code in file_io_utils. What do you think? CC: @rbrunner7 @hyc

Guilty ... I knew that, but when I wrote that code for #3313 I was on my last legs after working so long on that UTF-8 filename stuff that I simply hacked it in the simplest possible way.

I agree that it should be replaced sooner or later.

Agreed, that's the safe way to do it.

moneromooo-monero · 2018-06-23T12:58:10Z

src/common/util.cpp

+    {
+      if (flock(m_fd, LOCK_EX | LOCK_NB) == -1)
+      {
+        close(m_fd);


This could do with a MERROR/MWARNING so weird stuff afterwards can be known to be coming from that.

moneromooo-monero · 2018-06-23T13:00:18Z

src/wallet/wallet2.cpp

  std::string buf;
  r = ::serialization::dump_binary(keys_file_data, buf);
  r = r && epee::file_io_utils::save_string_to_file(keys_file_name, buf); //and never touch wallet_keys_file again, only read
  CHECK_AND_ASSERT_MES(r, false, "failed to generate wallet keys file " << keys_file_name);
+  m_keys_file_locker.reset(new tools::file_locker(m_keys_file.c_str()));


This uses c_str, but the ctor uses std::string, so this creates a new temporary string for no reason.
Having the ctor use const char * seems the way to support both without temp object creation.

moneromooo-monero · 2018-06-23T13:00:45Z

src/wallet/wallet2.cpp


+  m_keys_file_locker.reset();


Maybe add a comment saying so ?

moneromooo-monero · 2018-06-24T07:42:27Z

src/common/util.cpp

+    {
+      if (flock(m_fd, LOCK_EX | LOCK_NB) == -1)
+      {
+        MERROR("Failed to lock " << filename);


Could we get errno please ? :)

stoffu · 2018-06-25T06:58:40Z

Code updated reflecting suggestions

…ocesses

1d17647 epee.string_tools: add conversion between UTF-8 and UTF-16 (stoffu) 59de6f8 util: add file_locker class (stoffu) 3d623a8 wallet: prevent the same wallet file from being opened by multiple processes (stoffu)

49dc78d util: fix mistakes made in #3994 (stoffu)

stoffu force-pushed the lock-keys-file branch from 93d7e87 to ea388d7 Compare June 21, 2018 13:36

stoffu commented Jun 21, 2018

View reviewed changes

moneromooo-monero reviewed Jun 23, 2018

View reviewed changes

stoffu force-pushed the lock-keys-file branch from ea388d7 to 5a82dfb Compare June 24, 2018 04:01

moneromooo-monero reviewed Jun 24, 2018

View reviewed changes

stoffu force-pushed the lock-keys-file branch 5 times, most recently from 07f5b68 to 4686fc4 Compare June 25, 2018 04:27

moneromooo-monero approved these changes Jun 25, 2018

View reviewed changes

stoffu added 3 commits June 28, 2018 09:40

epee.string_tools: add conversion between UTF-8 and UTF-16

1d17647

util: add file_locker class

59de6f8

wallet: prevent the same wallet file from being opened by multiple pr…

3d623a8

…ocesses

stoffu force-pushed the lock-keys-file branch from 4686fc4 to 3d623a8 Compare June 28, 2018 00:41

luigi1111 merged commit 3d623a8 into monero-project:master Jun 28, 2018

stoffu added a commit to stoffu/monero that referenced this pull request Jun 28, 2018

util: fix mistakes made in monero-project#3994

49dc78d

luigi1111 added a commit that referenced this pull request Jun 28, 2018

Merge pull request #4069

65c09be

49dc78d util: fix mistakes made in #3994 (stoffu)

ghost mentioned this pull request Oct 20, 2018

wallet-rpc: Write subaddresses to wallet file instantly when created #4673

Closed

stoffu added a commit to stoffu/monero that referenced this pull request Nov 6, 2018

util: fix mistakes made in monero-project#3994 /monero#4069

490f23f

Doy-lee mentioned this pull request Nov 21, 2018

Multiple autostakes on the same wallet don't work oxen-io/oxen-core#333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: prevent the same wallet file from being opened by multiple processes #3994

wallet: prevent the same wallet file from being opened by multiple processes #3994

stoffu commented Jun 12, 2018

moneromooo-monero commented Jun 20, 2018

hyc commented Jun 20, 2018

stoffu commented Jun 21, 2018

stoffu Jun 21, 2018

moneromooo-monero Jun 23, 2018

stoffu Jun 24, 2018

moneromooo-monero Jun 23, 2018

stoffu Jun 24, 2018

rbrunner7 Jun 24, 2018 •

edited

Loading

hyc Jun 24, 2018

moneromooo-monero Jun 23, 2018

stoffu Jun 24, 2018

moneromooo-monero Jun 23, 2018

stoffu Jun 24, 2018

moneromooo-monero Jun 23, 2018

moneromooo-monero Jun 24, 2018

stoffu Jun 25, 2018

stoffu commented Jun 25, 2018

	static int utf8_to_utf16(const char src, int srcsize, wchar_t dst, int dstsize)
	{
	int need;
	wchar_t *result;
	need = MultiByteToWideChar(CP_UTF8, 0, src, srcsize, NULL, 0);
	if (need == 0xFFFD)
	return EILSEQ;
	if (need == 0)
	return EINVAL;
	result = malloc(sizeof(wchar_t) * need);
	if (!result)
	return ENOMEM;
	MultiByteToWideChar(CP_UTF8, 0, src, srcsize, result, need);
	if (dstsize)
	*dstsize = need;
	*dst = result;
	return 0;
	}

	LPCWSTR String::AnsiToUtf16(const char* ansi) {
	if (!ansi) return NULL;
	const int length = strlen(ansi);
	const int unicode_length =
	MultiByteToWideChar(CP_ACP, 0, ansi, length,
	NULL, 0);
	WCHAR* unicode = new WCHAR[unicode_length + 1];
	MultiByteToWideChar(CP_ACP, 0, ansi, length,
	unicode, unicode_length);
	unicode[unicode_length] = 0;
	return unicode;
	}

wallet: prevent the same wallet file from being opened by multiple processes #3994

wallet: prevent the same wallet file from being opened by multiple processes #3994

Conversation

stoffu commented Jun 12, 2018

moneromooo-monero commented Jun 20, 2018

hyc commented Jun 20, 2018

stoffu commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbrunner7 Jun 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu commented Jun 25, 2018

rbrunner7 Jun 24, 2018 •

edited

Loading