Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

bug: Don't try to handle SIGINT when prompting for passwords#1498

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
dwahler merged 13 commits intomainfromdavid/1378-terminal-state
May 18, 2022

Conversation

dwahler
Copy link
Contributor

This PR disables the SIGINT handler incliui.Prompt when theSecret option is set. This handler tries to run at the same time as the one installed byspeakeasy.Ask, and if it wins the race condition, it causes the program to exit before speakeasy can correctly restore the terminal settings.

We keep the signal handler when prompting for non-password inputs so that we can print a newline and give the user a clean prompt after we exit.

Fixes#1378

pty/pty.go Outdated
Comment on lines 31 to 32
// The file descriptor of the underlying PTY, if available
PTYFile()*os.File
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You could have methods for input and output file instead of a single method, which would make it Windows compatible. And then for the file descriptor just have another method for that as well (the windows.Handle is a file descriptor and can be cast to uintptr)

dwahler reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It might be cleaner to expose thereadWriter interface and add aFile function to it!readWriter could be renamed topipe, since that's pretty much what it's doing.

dwahler reacted with thumbs up emoji
Comment on lines 42 to 53
termios,err:=unix.IoctlGetTermios(int(ptty.PTY.PTYFile().Fd()),unix.TCGETS)
require.NoError(t,err)
require.Zero(t,termios.Lflag&unix.ECHO,"echo is on while reading password")

err=cmd.Process.Signal(os.Interrupt)
require.NoError(t,err)
_,err=cmd.Process.Wait()
require.NoError(t,err)

termios,err=unix.IoctlGetTermios(int(ptty.PTY.PTYFile().Fd()),unix.TCGETS)
require.NoError(t,err)
require.NotZero(t,termios.Lflag&unix.ECHO,"echo is off after reading password")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It would be good to see a Windows test too, shouldn't be too hard to get the console mode:

varstateuint32err:=windows.GetConsoleMode(handle,&state)iferr!=nil {returnerr}

@dwahler
Copy link
ContributorAuthor

I spent way too much time today trying to get this test to run on Windows and wasn't successful, so unless someone else has the time or the deep knowledge of Win32 to make it work, I think we'll have to be satisfied with testing on Linux.

As far as I've been able to determine, the problem boils down to a core difference between Unix pty devices and Windows ConPTY. On Linux, the kernel creates "master" and "slave" pty devices as a pair, and they behave as though they're connected by a bidirectional pipe. The slave device is what keeps track of the local echo state, and although the simplest thing you would do with it is let a child process inherit it as its TTY, there's nothing stopping the parent process from also keeping a reference to it and monitoring its state.

But on Windows, there are at leastthree distinct sets of handles:

  1. A pair of pipes created by the parent; the parent holds onto one end of each of these, and they play roughly the same role as a Unix "master" pty
  2. A "pseudoconsole handle" which the parent passes as a parameter toCreateProcess, encapsulating the other ends of the pipes (corresponding to data read/written by the child)
  3. A "console input buffer" and "console screen buffer" handle, which the child process receives as its stdio handles

Of these, 3 seems to be the only one that supportsGetConsoleMode which allows you to query the echo status. (The pipes are just streams of bytes that happen to contain terminal escape codes, and the pseudoconsole handle is an opaque reference that isn't actually usable for I/O.) But there seems to be no good way for the parent to use 1 or 2 to get a reference to 3. The child's handles are created automatically at process creation time, without the parent ever being able to see them.

see also:

https://devblogs.microsoft.com/commandline/windows-command-line-introducing-the-windows-pseudo-console-conpty/

microsoft/terminal#262 (comment)

It might be that testing this on Windows would require a totally different approach.

@kylecarbs
Copy link
Member

@dwahler I went down a similar rabbit hole with ConPTY when creating thepty package. Even our PTYs on Windows in testing aren'treal PTYs, because ConPTY requires attaching to a command.

@dwahlerdwahlerenabled auto-merge (squash)May 18, 2022 15:10
@dwahlerdwahler merged commit5f21a14 intomainMay 18, 2022
@dwahlerdwahler deleted the david/1378-terminal-state branchMay 18, 2022 15:26
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@deansheatherdeansheatherdeansheather approved these changes

@kylecarbskylecarbsAwaiting requested review from kylecarbs

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: CLI does not properly restore terminal state if interrupted during password input

3 participants

@dwahler@kylecarbs@deansheather

[8]ページ先頭

©2009-2025 Movatter.jp