Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSON corruption due concurrent file write #258

Open
vird opened this issue Nov 18, 2019 · 4 comments
Open

JSON corruption due concurrent file write #258

vird opened this issue Nov 18, 2019 · 4 comments

Comments

@vird
Copy link
Contributor

vird commented Nov 18, 2019

Due internal actions of edge wallet it can corrupt WalletName.json
sample of corrupted file
{"encryptionType":0,"iv_hex":"KmI7"}nbccfS5KIQIGo9k2iPW3Zp8pxV2Zg4XGo5sG3qHCU="}

You can look at really similar problem here nodejs/node#7978
Problem is located here https://github.com/EdgeApp/edge-core-js/blob/master/src/core/storage/repo.js#L78
We can call saveChanges before other saveChanges is occurs and now disklet comes...
It has relatively slow file write https://github.com/EdgeApp/disklet/blob/master/src/backends/node.ts#L89

So it tries to write, can't, tries to make directory and write one more time.
That's enough for fast second saveChanges call.
So first deepWriteFile made directory and tries to write
second saveChanges call calls deepWriteFile and tries to write too.

@vird
Copy link
Contributor Author

vird commented Nov 18, 2019

Proof.

/**
 * This will save a change-set into the local storage.
 * This function ignores folder-level deletes and overwrites,
 * but those can't happen under the current rules anyhow.
 */
var fireCount = 0;
export function saveChanges(
	disklet,
	changes
) {
	if (fireCount) {
		console.log("FIRE!!!");
		process.exit(1);
	}
	fireCount++;
	return Promise.all(
		Object.keys(changes).map(path => {
			const json = changes[path]
			return json != null
				? disklet.setText(path, JSON.stringify(json))
				: disklet.delete(path)
		})
	).then(()=>{
		fireCount--;
	})
}

image
Note I've removed some probably condidential info from screenshot

@vird
Copy link
Contributor Author

vird commented Nov 18, 2019

Also I've found that up to 4 concurrent saveChanges calls was in progress when 5th started.

if (fireCount > 0) {
    console.log("FIRE!!!", fireCount);
}

image

@vird
Copy link
Contributor Author

vird commented Nov 18, 2019

fix proposal vird@887b5bc
Note not MR yet, because I didn't get what flow errors means :-(

@swansontec
Copy link
Contributor

Yikes! Good catch.

If the Disklet node.js backend really has this problem, then it would affect other areas like login and blockchain syncing, as well. I think the proper fix needs to happen in Disklet itself, rather than the sync module, since that will fix everything at once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants