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

simplewallet: Add Unicode input_line [Ryo backport] #4390

Merged
merged 2 commits into from Oct 5, 2018

Conversation

@fireice-uk
Copy link
Contributor

@fireice-uk fireice-uk commented Sep 16, 2018

Issue description : ryo-currency/ryo-currency#102
Monero Issue : #4351

@@ -36,6 +36,14 @@
#include "cryptonote_config.h"
#include "string_tools.h"

#ifdef WIN32
Copy link
Collaborator

@moneromooo-monero moneromooo-monero Sep 16, 2018

Please use _WIN32 (apparently WIN32 is only defined if you've got the right include included already, so it's error prone)

Copy link
Contributor Author

@fireice-uk fireice-uk Sep 16, 2018

What you are thinking is MSVC defines (not that the code compiles there anyway), but sure.

Copy link
Collaborator

@moneromooo-monero moneromooo-monero Sep 16, 2018

No idea about windows. I'm repeating what I was told by someone who does know, but maybe I'm assuming things. If you think that is wrong, explain why.

Copy link
Contributor Author

@fireice-uk fireice-uk Sep 17, 2018

To put it in simple terms:

gcc defines WIN32 and _WIN32
msvc defines _WIN32

You use WIN32 (and I did out of laziness), however you suggestion is better practice.

int size_needed = WideCharToMultiByte(CP_UTF8, 0, buffer, -1, NULL, 0, NULL, NULL);
std::string buf(size_needed, '\0');
WideCharToMultiByte(CP_UTF8, 0, buffer, -1, &buf[0], size_needed, NULL, NULL);
buf.pop_back(); //size_needed includes null that we needed to have space for
Copy link
Collaborator

@moneromooo-monero moneromooo-monero Sep 16, 2018

Can you move that inner code to util please, it seems generally useful. It also has nothing to do with command line. Without the trim seems best. Then you can have the input_line kept in simplewallet calling it and trimming. Also gets rid of all the diff spam due to namespaces below.

Copy link
Contributor Author

@fireice-uk fireice-uk Sep 16, 2018

What do you mean by inner code -> UCS2 conversion? Or do you want the read_line to act as a wrapper on win?

Copy link
Collaborator

@moneromooo-monero moneromooo-monero Sep 16, 2018

I mean the whole code except the trim, which is simplewallet specific, as it's not a canonical way to behave for a line reading function. So simplewallet would keep a function that calls the utils input function, then trims.

buffer[read] = 0;

SetConsoleMode(hConIn, oldMode);
CloseHandle(hConIn);
Copy link
Collaborator

@moneromooo-monero moneromooo-monero Sep 16, 2018

Any of those return error codes that need checking ?

Copy link
Contributor Author

@fireice-uk fireice-uk Sep 16, 2018

I subscribe to school that return codes should be checked if something can be done about it. Here the only thing available would be to throw.

@fireice-uk fireice-uk force-pushed the topic-unicode-support branch from c43d896 to 7852402 Sep 17, 2018
@fireice-uk
Copy link
Contributor Author

@fireice-uk fireice-uk commented Sep 17, 2018

Let me know if that's what you meant. Note I made a reply on the collapsed topic here : #4390 (comment)

@moneromooo-monero
Copy link
Collaborator

@moneromooo-monero moneromooo-monero commented Sep 17, 2018

Yes, that is what I meant. Note that seeds are input via input_secure_line, not input_line, so that one will also have to be changed.

@fireice-uk
Copy link
Contributor Author

@fireice-uk fireice-uk commented Sep 17, 2018

Looks like something you merged after the base of this PR. Hold on, let me rebase then.

@fireice-uk fireice-uk force-pushed the topic-unicode-support branch from 7852402 to e08ab7d Sep 17, 2018
@fireice-uk
Copy link
Contributor Author

@fireice-uk fireice-uk commented Sep 17, 2018

Ok, looks like someone already did most of the work on that one for me, apart from failing to call the correct API (ReadConsoleW), that should be ez.

One note the existing code. UTF8 characters can be up to 4 bytes long. This kind of thing makes people scream in agony:

char ch; r = (TRUE == ::ReadConsoleA(h_cin, &ch, 1, &read, NULL));

@@ -56,8 +56,6 @@ namespace

bool read_from_tty(epee::wipeable_string& pass, bool hide_input)
{
static constexpr const char BACKSPACE = 8;
Copy link
Contributor Author

@fireice-uk fireice-uk Sep 17, 2018

I have no idea why someone chose to do this instead of using \b

Copy link
Collaborator

@hyc hyc Sep 28, 2018

Most likely for symmetry with the Unix routine (which used BACKSPACE = 127, ASCII DEL). I'd suggest preserving this line. Although I would have named them both DELETE, not BACKSPACE.

@fireice-uk
Copy link
Contributor Author

@fireice-uk fireice-uk commented Sep 17, 2018

Ok. Done. The second commit adds proper handling of multibyte characters.

Note: Please check whether the code actually builds, I wrote it totally "dry" on Linux.

@moneromooo-monero
Copy link
Collaborator

@moneromooo-monero moneromooo-monero commented Sep 17, 2018

Ah, my mistake. I had been assuming you'd actually tried it at least once. If not, then we'll wait some more for a Windows coder to show up and do it.

@fireice-uk
Copy link
Contributor Author

@fireice-uk fireice-uk commented Sep 17, 2018

That's fine - if you are short on manpower I can do it tomorrow evening. It is purely a matter of me having to power down and switch to Windows.

@moneromooo-monero
Copy link
Collaborator

@moneromooo-monero moneromooo-monero commented Sep 17, 2018

OK, that's fine. It's not like we're in a hurry about this now.

@fireice-uk fireice-uk force-pushed the topic-unicode-support branch from 3b2c819 to 56cbf00 Sep 19, 2018
@fireice-uk
Copy link
Contributor Author

@fireice-uk fireice-uk commented Sep 19, 2018

@moneromooo-monero Compiled it on win and fixed two minor build errors. I think that's everything from my end.

if((len = WideCharToMultiByte(CP_UTF8, 0, &ucs2_ch, 1, utf8_ch, sizeof(utf8_ch), NULL, NULL)) <= 0)
break;

chlen.push_back(len);
Copy link
Collaborator

@hyc hyc Sep 28, 2018

It's possible for the UTF8 char to push pass.size() over the max_password_size. Should check the length and break out of the loop here, before appending the new character.

I think it's bad practice to just silently return with whatever was read so far, but the existing code already does this. And I suppose the likelihood of anyone entering such a long password is low to begin with, so maybe not a serious problem.

HANDLE hConIn = CreateFileW(L"CONIN$", GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, 0, nullptr);
DWORD oldMode;

FlushConsoleInputBuffer(hConIn);
Copy link
Collaborator

@hyc hyc Sep 28, 2018

Why can't we just use STD_INPUT_HANDLE here?

The Unix version will work with input redirection. I don't see why we should break that for Windows.

@@ -187,9 +146,10 @@ namespace
std::cout << prompt;

std::string buf;
#ifdef _WIN32
buf = tools::input_line_win();
Copy link
Collaborator

@hyc hyc Sep 28, 2018

Probably should have emulated std::getline() semantics instead, e.g.
tools::getline(buf);

Copy link
Contributor Author

@fireice-uk fireice-uk Oct 4, 2018

This paradigm is actually faster in C++11 onwards (copy elision).

@moneromooo-monero
Copy link
Collaborator

@moneromooo-monero moneromooo-monero commented Oct 1, 2018

The buffer overflow should be fixed at least. If you want this in 0.13, fix it quick please.

@fireice-uk
Copy link
Contributor Author

@fireice-uk fireice-uk commented Oct 4, 2018

@moneromooo-monero reserve is purely a memory optimisation - a C++ vector is dynamically sized (that's kind of the point), so there is no overflow here - not sure about any other code. I can add a break if it is exceeded if you want though, it is two lines after-all =).

@fireice-uk fireice-uk force-pushed the topic-unicode-support branch from 56cbf00 to 8823d78 Oct 4, 2018
@fireice-uk
Copy link
Contributor Author

@fireice-uk fireice-uk commented Oct 4, 2018

Let me know if that's what you meant. Whenever dealing with Unicode you should really delete all size variables. You need to be specific whether you mean bytes, characters or glyphs since the count of all three can be different.

if((len = WideCharToMultiByte(CP_UTF8, 0, &ucs2_ch, 1, utf8_ch, sizeof(utf8_ch), NULL, NULL)) <= 0)
break;

if(std::accumulate(chlen.begin(), chlen.end(), 0) + len >= tools::password_container::max_password_size)
Copy link
Collaborator

@hyc hyc Oct 4, 2018

if(pass.size() + len >= tools::password_container::max_password_size)

Copy link
Contributor Author

@fireice-uk fireice-uk Oct 4, 2018

Updated

@hyc
Copy link
Collaborator

@hyc hyc commented Oct 4, 2018

In this context we have to assume size means bytes.

@moneromooo-monero
Copy link
Collaborator

@moneromooo-monero moneromooo-monero commented Oct 4, 2018

Could be, I didn't look at the code hyc pointed out. If there's a bug, then it needs fixing. If it's just a misunderstanding, then it can be merged now.

@fireice-uk fireice-uk force-pushed the topic-unicode-support branch from 8823d78 to a061353 Oct 4, 2018
Copy link
Collaborator

@fluffypony fluffypony left a comment

Reviewed

@fluffypony fluffypony merged commit a061353 into monero-project:master Oct 5, 2018
2 of 10 checks passed
fluffypony added a commit that referenced this issue Oct 5, 2018
a061353 secure_pwd_reader: Add proper Unicode handling [Ryo contribution] (fireice-uk)
579383c simplewallet: Add Unicode input_line [Ryo backport] (fireice-uk)
fluffypony added a commit that referenced this issue Oct 6, 2018
a061353 secure_pwd_reader: Add proper Unicode handling [Ryo contribution] (fireice-uk)
579383c simplewallet: Add Unicode input_line [Ryo backport] (fireice-uk)
fotolockr pushed a commit to fotolockr/monero that referenced this issue May 1, 2019
a061353 secure_pwd_reader: Add proper Unicode handling [Ryo contribution] (fireice-uk)
579383c simplewallet: Add Unicode input_line [Ryo backport] (fireice-uk)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants