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

reflect: Select panics if array length greater than 1<<16 #37350

Closed
thempatel opened this issue Feb 21, 2020 · 7 comments
Closed

reflect: Select panics if array length greater than 1<<16 #37350

thempatel opened this issue Feb 21, 2020 · 7 comments

Comments

@thempatel
Copy link

@thempatel thempatel commented Feb 21, 2020

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

$ go version
1.13.7

Does this issue reproduce with the latest release?

Yes, here is a go playground example

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/milanpatel/Library/Caches/go-build"
GOENV="/Users/milanpatel/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/milanpatel/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.7/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.7/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/gx/lcfzy_f56tlb_czrp7z2ptyc0000gn/T/go-build327901560=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

We ran into an issue today where if the number of reflect cases in an array exceeds 1<<16, the call to reflect.Select panics. The function doc for the select implementation suggests that it's the length of the array that is used to recreate the pointer to the array, determined by a passed in variable ncases.

// selectgo implements the select statement.
//
// cas0 points to an array of type [ncases]scase, and order0 points to
// an array of type [2*ncases]uint16. Both reside on the goroutine's
// stack (regardless of any escaping in selectgo).
//
// selectgo returns the index of the chosen scase, which matches the
// ordinal position of its respective select{recv,send,default} call.
// Also, if the chosen scase was a receive operation, it reports whether
// a value was received.
func selectgo(cas0 *scase, order0 *uint16, ncases int) (int, bool) {

In fact, that doesn't appear to be true:

	cas1 := (*[1 << 16]scase)(unsafe.Pointer(cas0))
	...
	scases := cas1[:ncases:ncases] // <- panics here 
@ianlancetaylor ianlancetaylor changed the title reflect.Select panics if array length greater than 1<<16 reflect: Select panics if array length greater than 1<<16 Feb 21, 2020
@ianlancetaylor ianlancetaylor added this to the Go1.15 milestone Feb 21, 2020
@balasanjay
Copy link
Contributor

@balasanjay balasanjay commented Feb 21, 2020

reflect.Select will likely be extremely expensive if you're passing 65k cases; it will have to sort the 65k channels, acquire 65k locks, register itself on 65k independent wait queues, etc.

It seems like a good thing that it panics at that scale?

(Maybe it should panic even sooner at some lower value)

Loading

@thempatel
Copy link
Author

@thempatel thempatel commented Feb 21, 2020

It's a fair point, I figured we were abusing the API. I'm pretty O-K if the solution to this ends up being to just update the function doc, it's not clear right now without looking at the code (i.e. no warning) that there's an upper limit to how many you can pass in. Our work around for this was pretty much to shard out the work across threads instead of using a mega thread with all the select cases.

WDYT?

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 21, 2020

If we do decide to panic on cases like this it for sure should not be a panic that just says "slice bounds out of range".

Personally I'm inclined to think that we should just support it. Yes, the performance will be bad. But relatively arbitrary limitations are bad in a different way. If the limit were billions I would feel differently. Thousands of cases doesn't seem inherently absurd.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 21, 2020

If we want to bump up the 1<<16 limit, we'll also have to bump up the type uint16. That will then increase stack usage by all functions that contain select statements.

Not a big deal, but wanted to make sure it's clear that 1<<16 here isn't an arbitrary restriction.

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 21, 2020

Ah, thanks, I hadn't noticed that.

Given that I'm OK with the restriction, and I just think that we need 1) documentation; 2) a better panic message.

Loading

@thempatel
Copy link
Author

@thempatel thempatel commented Feb 21, 2020

Given that I'm OK with the restriction, and I just think that we need 1) documentation; 2) a better panic message.

Agree, happy to make the change!

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 23, 2020

Change https://golang.org/cl/220583 mentions this issue: reflect: update Select to panic early on excessive input cases

Loading

@gopherbot gopherbot closed this in 7802b55 Feb 24, 2020
@golang golang locked and limited conversation to collaborators Feb 23, 2021
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