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

x/crypto/ssh/terminal: Repeats calls to terminal.ReadPassword causes scanner to produce "read /dev/stdin: The handle is invalid" #23525

Closed
kettle11 opened this issue Jan 23, 2018 · 8 comments
Milestone

Comments

@kettle11
Copy link

@kettle11 kettle11 commented Jan 23, 2018

What version of Go are you using (go version)?

go1.9.1 windows/amd64

Does this issue reproduce with the latest release?

I haven't tested this yet.

What operating system and processor architecture are you using (go env)?

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\ikett\go
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config

What did you do?

https://play.golang.org/p/zaYMheWt9fe

package main

import (
	"bufio"
	"fmt"
	"os"
	"syscall"

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

func main() {
	scanner := bufio.NewScanner(os.Stdin)
	i := 0
	for {
		scanner.Scan()

		err := scanner.Err()
		if err != nil {
			fmt.Println(scanner.Err())
			return
		}

		fmt.Println("Test", i)
		i++
		terminal.ReadPassword(int(syscall.Stdin))
	}
}

Run this code and press enter about 30-60 times. Eventually the scanner produces the following error: read /dev/stdin: The handle is invalid.

In my actual project code this error occurred after fewer repetitions (3-4), but this minimum code set requires more repetitions for some reason.

What did you expect to see?

I expected the scanner to continue to work and not produce an error.

What did you see instead?

It produced " read /dev/stdin: The handle is invalid. " and stopped working.

@kettle11 kettle11 changed the title Repeats calls to terminal.ReadPassword causes scanner to produce "read /dev/stdin: The handle is invalid" x/crypto: Repeats calls to terminal.ReadPassword causes scanner to produce "read /dev/stdin: The handle is invalid" Jan 23, 2018
@gopherbot gopherbot added this to the Unreleased milestone Jan 23, 2018
@vcabbage
Copy link
Member

@vcabbage vcabbage commented Jan 24, 2018

I saw this question come up on Gopher's Slack and theorized that the use of os.NewFile() here is causing a finalizer to be set and eventually closing stdin.

@kettle11
Copy link
Author

@kettle11 kettle11 commented Jan 24, 2018

I just verified vcabbage's hypothesis by manually calling runtime.GC() and the error immediately occurs after the first call to scanner.Scan() after terminal.ReadPassword and runtime.GC() are invoked.

Check out this new example code:

package main

import (
	"bufio"
	"fmt"
	"os"
	"runtime"
	"syscall"

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

func main() {
	scanner := bufio.NewScanner(os.Stdin)
	i := 0
	for {
		scanner.Scan()

		err := scanner.Err()
		if err != nil {
			fmt.Println(scanner.Err())
			return
		}

		i++
		terminal.ReadPassword(int(syscall.Stdin))
		runtime.GC()
	}
}
@vcabbage
Copy link
Member

@vcabbage vcabbage commented Jan 24, 2018

Looks like this was broken in https://golang.org/cl/79335, which was a fix for #22828.

/cc @alexbrainman @mattn

@mattn
Copy link
Member

@mattn mattn commented Jan 24, 2018

Seems to be that the handle of STDIN is closed by GC. I'll send CL soon.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 24, 2018

Change https://golang.org/cl/89395 mentions this issue: crypto/ssh/terminal: close stdin handle

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Jan 24, 2018

@kettle11 does https://go-review.googlesource.com/#/c/crypto/+/89395/ fixes your problem?

Thank you.

Alex

@kettle11
Copy link
Author

@kettle11 kettle11 commented Jan 24, 2018

@alexbrainman @mattn I just applied and tested the fix locally and it works for me!

Thanks @mattn and @vcabbage for looking into it!

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Jan 25, 2018

I just applied and tested the fix locally and it works for me!

Thanks for checking @kettle11

Alex

@mikioh mikioh changed the title x/crypto: Repeats calls to terminal.ReadPassword causes scanner to produce "read /dev/stdin: The handle is invalid" x/crypto/ssh/terminal: Repeats calls to terminal.ReadPassword causes scanner to produce "read /dev/stdin: The handle is invalid" Jan 26, 2018
@golang golang locked and limited conversation to collaborators Jan 26, 2019
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
os.NewFile assigns finalizer to close file handle
passed into ReadPassword. But that is not expected.
Make a duplicate of original file handle, and pass
copy handle into ReadPassword instead.

Fixes golang/go#23525

Change-Id: I4d6725e9a1cc20defd1b58afc383e35a7f9ee4e9
Reviewed-on: https://go-review.googlesource.com/89395
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
os.NewFile assigns finalizer to close file handle
passed into ReadPassword. But that is not expected.
Make a duplicate of original file handle, and pass
copy handle into ReadPassword instead.

Fixes golang/go#23525

Change-Id: I4d6725e9a1cc20defd1b58afc383e35a7f9ee4e9
Reviewed-on: https://go-review.googlesource.com/89395
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
os.NewFile assigns finalizer to close file handle
passed into ReadPassword. But that is not expected.
Make a duplicate of original file handle, and pass
copy handle into ReadPassword instead.

Fixes golang/go#23525

Change-Id: I4d6725e9a1cc20defd1b58afc383e35a7f9ee4e9
Reviewed-on: https://go-review.googlesource.com/89395
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
umk pushed a commit to umk/go-terminal that referenced this issue Nov 10, 2019
os.NewFile assigns finalizer to close file handle
passed into ReadPassword. But that is not expected.
Make a duplicate of original file handle, and pass
copy handle into ReadPassword instead.

Fixes golang/go#23525

Change-Id: I4d6725e9a1cc20defd1b58afc383e35a7f9ee4e9
Reviewed-on: https://go-review.googlesource.com/89395
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit to golang/term that referenced this issue Oct 16, 2020
os.NewFile assigns finalizer to close file handle
passed into ReadPassword. But that is not expected.
Make a duplicate of original file handle, and pass
copy handle into ReadPassword instead.

Fixes golang/go#23525

Change-Id: I4d6725e9a1cc20defd1b58afc383e35a7f9ee4e9
Reviewed-on: https://go-review.googlesource.com/89395
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.