Suggest a patch to handle ^C interrupts in ReadPassword from golang.org/x/crypto/ssh/termina/util.go

[Joe Linoff]

Hi,

I recently ran into a minor problem with ReadPassword() in golang.org/x/crypto/ssh/terminal.

When a user types ^C while entering the password, terminal echo is not restored because the program exits immediately which means that the defer syscall is never executed. This leaves the  terminal a no-echo state which is confusing for users.

I did not submit this as an issue because I am not sure if this is the desired behavior but for my users it was problematic so I added the following simple workaround to capture interrupts and correctly reset terminal echo.

    // pre-existing code (line 101: https://github.com/golang/crypto/blob/master/ssh/terminal/util.go)
    defer func() {
        syscall.Syscall6(syscall.SYS_IOCTL, uintptr(fd), ioctlWriteTermios, uintptr(unsafe.Pointer(&oldState)), 0, 0, 0)
    }()

    // Added this modification to handle a ^C interrupt.
    // Make sure that we reset term echo before exiting.
    sc := make(chan os.Signal, 1)
    signal.Notify(sc, os.Interrupt)
    go func() {
        for _ = range sc {
            syscall.Syscall6(syscall.SYS_IOCTL, uintptr(fd), ioctlWriteTermios, uintptr(unsafe.Pointer(&oldState)), 0, 0, 0)
            os.Exit(1)
        }
    }()

    // pre-existing code (line 105: https://github.com/golang/crypto/blob/master/ssh/terminal/util.go)
    var buf [16]byte
    var ret []byte

I have tested this workaround on linux (CentOS and Ubuntu) and on Mac OS X using go-1.6.2 for terminal interactions. I have not tested it for pipes.

Should I submit this as an issue?

Regards,

Joe

[Dave Cheney]

No thank you. The code that changes the echo mode should be in charge of resetting it.

[Tim K]

I believe that's exactly what he is suggesting, adding an INT signal handler in addition to the existing defer.

[Dave Cheney]

I'm sorry I missed that part. I still don't think this method should attach a signal handler. That should be the property of the main package for whatever program uses this package.

[James Chacon]

That's an odd interaction/expectation to expect of the main package. i.e. "if you use this package you really should setup a signal handler to reset the terminal in case of INTR".

James

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Konstantin Khomoutov]

On Mon, 6 Jun 2016 07:48:54 -0700
James Chacon <chacon...@gmail.com> wrote:

> That's an odd interaction/expectation to expect of the main package.
> i.e. "if you use this package you really should setup a signal
> handler to reset the terminal in case of INTR".

No, the basic idea is that any library package has no business managing
the global state of the application, and signal handling is the global
state.

[Joe Linoff]

What would be the best way to re-enable terminal echo in main? Is there an exported terminal interface to do that?

Thanks

[Joe Linoff]

Doesn't the current implementation change the global state now? If you type ^C in the current implementation, the application exits but it leaves the terminal in a different state than it started with.

[James Chacon]

I agree but you're suggesting conflicting arguments here. Evidently golang.org/x/crypto/ssh/terminal is allowed to change global state (terminal echo) but isn't allowed to then setup a mechanism to replace that state upon abnormal exit?

It's either one or the other here. Either it should be enabling a signal handler (with chaining) to do proper cleanup or it shouldn't be modifying terminal state at all and a caller must put it in non-echo mode and take it out.

James

[Manlio Perillo]

There is a problem, however.

In order to reset the old terminal status, the main package needs to know the old state; however this is only know to the library.

One good thing of signal handling in the Go runtime is that every registered channel receive the signal, but this still does not make it a good solution.

In order to solve problems like this, some time ago I wrote github.com/perillo/atexit.

It is intended to complement standard defer mechanism when a function terminates abnormally.

The library package register a deferred function with atexit, and the main package calls atexit.Exit when a signal is received.

Manlio

[Manlio Perillo]

Il giorno lunedì 6 giugno 2016 18:42:30 UTC+2, James Chacon ha scritto:

On Mon, Jun 6, 2016 at 8:08 AM, Konstantin Khomoutov <flat...@users.sourceforge.net> wrote:

On Mon, 6 Jun 2016 07:48:54 -0700
James Chacon <chacon...@gmail.com> wrote:

> That's an odd interaction/expectation to expect of the main package.
> i.e. "if you use this package you really should setup a signal
> handler to reset the terminal in case of INTR".

No, the basic idea is that any library package has no business managing
the global state of the application, and signal handling is the global
state.

I agree but you're suggesting conflicting arguments here. Evidently golang.org/x/crypto/ssh/terminal is allowed to change global state (terminal echo) but isn't allowed to then setup a mechanism to replace that state upon abnormal exit?

It's either one or the other here. Either it should be enabling a signal handler (with chaining) to do proper cleanup

> [...]

The problem is that the library function does know what signals it should be handle.  Moreover some goroutine may cause program termination, and the cleanup code will not be called.

Manlio 

[James Chacon]

"The program may terminate" is a nice strawman but irrelevant. Under normal circumstances (i.e. not crashing) the library should be returning the state of the system to how it started. This means handling normal termination (i.e. ^C/INTR situations) cleanly too. Only the library knows what it did here so main can't reasonably be expected to deal with half state scenarios.

James

[Manlio Perillo]

On Mon, Jun 6, 2016 at 8:00 PM, James Chacon <chacon...@gmail.com> wrote:
> [...]

>>> I agree but you're suggesting conflicting arguments here. Evidently
>>> golang.org/x/crypto/ssh/terminal is allowed to change global state (terminal
>>> echo) but isn't allowed to then setup a mechanism to replace that state upon
>>> abnormal exit?
>>>
>>> It's either one or the other here. Either it should be enabling a signal
>>> handler (with chaining) to do proper cleanup
>>
>>
>> > [...]
>>
>> The problem is that the library function does know what signals it should
>> be handle. Moreover some goroutine may cause program termination, and the
>> cleanup code will not be called.
>>
>
> "The program may terminate" is a nice strawman but irrelevant. Under normal
> circumstances (i.e. not crashing) the library should be returning the state
> of the system to how it started. This means handling normal termination
> (i.e. ^C/INTR situations) cleanly too. Only the library knows what it did
> here so main can't reasonably be expected to deal with half state scenarios.
>

By normal termination I mean when the program terminates by returning from main.
In this case each each function executed in the program returns, and
deferred are called.

By abnormal termination I mean when some goroutine calls os.Exit.
In this case functions running concurrently at the time os.Exit is
called will not have deferred functions called.

Now, when you handle a signal, you usually call os.Exit in a goroutine
(unless you want to ignore the signal).
But the same may happen when some goroutine (defined in the main
package, I hope) calls, as an example, log.Fatal.

The ReadPassword function simply does not have this information.
Registering a SIGINT handler does not solve the problem in the general
case, unless the program have only one goroutine.


Manlio

[James Chacon]

Can it handle all program terminations? No, but nothing can because you can always be killed via SIGKILL.

But should it handle the obvious cases? Yes, especially since this one is fairly common. The whole point of a password prompting library is generally a CLI accepting user input. Those get terminated fairly often with ^C when testing/etc so having to then blind reset your terminal isn't terribly friendly. Nor is it friendly to expect every caller to reinvent this wheel since it's fairly specific to this library behavior in changing terminal state. 

James

[Konstantin Shaposhnikov]

The problem with ReadPassword installing an interrupt signal handler is that the calling program has no way to catch Interrupt signal and not terminate after ReadPassword is called. A library function should not make implementing such behavior impossible.

I recently had to solve the same problem recently and came up with the following code:

package main

import (

"os"

"os/signal"

"syscall"

"golang.org/x/crypto/ssh/terminal"

)

var initialState *terminal.State

func init() {

// remember initial terminal state

var err error

if initialState, err = terminal.GetState(syscall.Stdin); err != nil {

return

}

// and restore it on exit

c := make(chan os.Signal)

signal.Notify(c, os.Interrupt, os.Kill)

go func() {

<-c

_ = terminal.Restore(syscall.Stdin, initialState)

os.Exit(0)

}()

}

 It works with Ctrl+C and kill (and even with kill -9). Hopefully this helps.

[Joe Linoff]

Thank you. This is a big help.

Joe

[Manlio Perillo]

On Tue, Jun 7, 2016 at 2:19 AM, Konstantin Shaposhnikov
<k.shapo...@gmail.com> wrote:
> The problem with ReadPassword installing an interrupt signal handler is that
> the calling program has no way to catch Interrupt signal and not terminate
> after ReadPassword is called.

What do you mean?
Both ReadPassword and the main function can call signal.Notify, and
both will receive a copy of the same signal.

What is the reason why Notify is not called in the init function?

> [...]

Manlio

[Art Mellor]

Can't we have both? Couldn't the package provide a function to allow the main app to indicate it wants cleanup? If the main app doesn't turn that on, then it behaves as it does currently. If it does, you get the cleanup. It wouldn't break existing functionality, just augment it.

[Joe Linoff]

Based on the example from Konstantin and other feedback on this thread, I modified my getPassword() function as follows:

// Correctly resets terminal echo after ^C interrupts.
package main

import (
    "fmt"

    "os"
    "os/signal"
    "syscall"
    "golang.org/x/crypto/ssh/terminal"
)

func main() {
    // Get password.
    p := getPassword("password: ")
    fmt.Println("value:", p)
}

func getPassword(prompt string) string {
    // Get the initial state of the terminal.
    initialTermState, e1 := terminal.GetState(syscall.Stdin)
    if e1 != nil {
        panic(e1)
    }

    // Restore it in the event of an interrupt.
    // CITATION: Konstantin Shaposhnikov - https://groups.google.com/forum/#!topic/golang-nuts/kTVAbtee9UA

    c := make(chan os.Signal)
    signal.Notify(c, os.Interrupt, os.Kill)
    go func() {
        <-

c
        _ = terminal.Restore(syscall.Stdin, initialTermState)
        os.Exit(1)
    }()

    // Now get the password.
    fmt.Print(prompt)
    p, err := terminal.ReadPassword(syscall.Stdin)
    fmt.Println("")
    if err != nil {
        panic(err)
    }

    // Stop looking for ^C on the channel.
    signal.Stop(c)

    // Return the password as a string.
    return string(p)
}