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

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
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.15

Comments

@thempatel
Copy link

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 21, 2020
@ianlancetaylor ianlancetaylor added this to the Go1.15 milestone Feb 21, 2020
@ianlancetaylor ianlancetaylor mentioned this issue Feb 21, 2020
Closed
@balasanjay
Copy link
Contributor

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)

@thempatel
Copy link
Author

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?

@ianlancetaylor
Copy link
Contributor

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.

@mdempsky
Copy link
Member

mdempsky commented Feb 21, 2020 •
edited

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.

@ianlancetaylor
Copy link
Contributor

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.

@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Feb 21, 2020
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 21, 2020
@thempatel
Copy link
Author

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!

@gopherbot
Copy link

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

@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.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
5 participants
@mdempsky @balasanjay @ianlancetaylor @gopherbot @thempatel