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

Jump to bottom

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
Closed

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

kettle11 opened this issue Jan 23, 2018 · 8 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@kettle11
Copy link

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

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 commented Jan 24, 2018 •
edited

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

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 commented Jan 24, 2018

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

@gopherbot
Copy link

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

@alexbrainman
Copy link
Member

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

Thank you.

Alex

@kettle11
Copy link
Author

@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

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
@mattn @alexbrainman
ssh/terminal: use duplicate handle in ReadPassword
22fb240 
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
@mattn @alexbrainman
ssh/terminal: use duplicate handle in ReadPassword
3b60acd 
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
@mattn @alexbrainman
ssh/terminal: use duplicate handle in ReadPassword
f77cc4b 
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
@mattn @alexbrainman
ssh/terminal: use duplicate handle in ReadPassword
189f313 
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>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@mattn @alexbrainman
ssh/terminal: use duplicate handle in ReadPassword
0ccb096 
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>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@mattn @alexbrainman
ssh/terminal: use duplicate handle in ReadPassword
1543870 
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>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@mattn @alexbrainman
ssh/terminal: use duplicate handle in ReadPassword
3963232 
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>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@mattn @alexbrainman
ssh/terminal: use duplicate handle in ReadPassword
4d17c5a 
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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@mattn @kettle11 @gopherbot @alexbrainman @vcabbage