Strip extraneous carriage return from end of entered password

[hiddentao]

Current users of the Mist wallet on OS X are unable to import their presale wallets. This is due to the fallback terminal password input mechanism failing to strip off the carriage return at the end of the input. I confirmed that this is a bug within geth itself.

This PR fixes this issue.

[robotally]

❗ Pull request against master ❗

Vote	Count	Reviewers
👍	0
👎	0

Updated: Tue Mar 22 09:18:51 UTC 2016

[codecov-io]

Current coverage is 48.50%

Merging #2369 into master will decrease coverage by -0.01% as of c701aaa

Powered by Codecov. Updated on successful CI builds.

[fjl]

This issue is also fixed in #2284 (and it adds a test to catch it).

[alexvandesande]

@fjl Your PR changes 75 files and addresses multiple issues at the same time. This is a one line fix that fixes a huge problem for the Wallet users. Can't we merge this independently without needing to wait for your PR?

[bas-vk]

Does this work on Windows since it uses \r\n as newline?

[hiddentao]

@bas-vk Good point. I'm on OS X. I should test it out on Windows.

On a larger note I'm wondering whether a better approach might be to modify the wallet import command to accept a supplied password in the initial command. I'm tempted to make that fix, especially since @fjl has better-tested fix which would then overwrite mine anyway.

[obscuren]

Thank you for your PR. Several notes:

1. PRs must be made against develop.
2. What if a user actually has \n at the end of their passwords

Stripping characters from password is not advised. We shouldn't assume people don't actually put \n at the end of their password.

[hiddentao]

Thanks, I'll make sure it's done against develop.

As for the second point, I did think about that. But from my limited understanding of the fallback terminal input code, it seems like it stops taking input once \n is encountered. So if the user was typing their password in via geth and it was using the fallback terminal input I don't see how they'd be able to supply a password containing \n anyway. Please correct me if I'm wrong.

In any case, as per my previous comment I think perhaps being able to supply the password directly as part of the import wallet invocation would be a better overall solution at least as far as Mist is concerned.

[fjl]

We cannot provide the password on the command line because the command line arguments are readable by other processes (e.g. through the /proc file system).

The long-term fix will be to move account management (and with it presale key importing‎) into Mist. This PR is fine as a short term fix and should be included in geth 1.3.6.

‎

[obscuren]

@hiddentao you're quite right, my theory is flawed.

This PR would be good as a temporary solution if it also addresses the same issue for windows.

I'll close this PR in favour of the PR to be made againstdevelop.

[hiddentao]

@fjl Good point on the command-line.

@obscuren I've raised #2371 accordingly.