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/text: panic in regionGroupDist #43834

Closed
sethvargo opened this issue Jan 21, 2021 · 9 comments
Closed

x/text: panic in regionGroupDist #43834

sethvargo opened this issue Jan 21, 2021 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@sethvargo
Copy link
Contributor

sethvargo commented Jan 21, 2021

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

$ go version
go version go1.15.6 darwin/amd64

Does this issue reproduce with the latest release?

Yes and HEAD too

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/sethvargo/Library/Caches/go-build"
GOENV="/Users/sethvargo/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/sethvargo/Development/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/sethvargo/Development/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.15.6/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.15.6/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/sethvargo/Development/text/go.mod"
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/cs/jc9pj94x493gb8jr49ys7cnc00gy5b/T/go-build275332944=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Attempted to ParseAcceptLanguage a non-existent language us-zz. There's a failing test as a PR at golang/text#17.

What did you expect to see?

Not a panic.

What did you see instead?

A panic.

panic: runtime error: index out of range [357] with length 357 [recovered]
	panic: runtime error: index out of range [357] with length 357

goroutine 357 [running]:
testing.tRunner.func1.1(0x138cec0, 0xc000494c60)
	/usr/local/Cellar/go/1.15.6/libexec/src/testing/testing.go:1072 +0x30d
testing.tRunner.func1(0xc0004c7380)
	/usr/local/Cellar/go/1.15.6/libexec/src/testing/testing.go:1075 +0x41a
panic(0x138cec0, 0xc000494c60)
	/usr/local/Cellar/go/1.15.6/libexec/src/runtime/panic.go:969 +0x1b9
golang.org/x/text/language.regionGroupDist(0x139005a00cf0165, 0x141b400)
	/Users/sethvargo/Development/text/language/match.go:676 +0x13b
golang.org/x/text/language.TestRegionGroups(0xc0004c7380)
	/Users/sethvargo/Development/text/language/match_test.go:178 +0x467
testing.tRunner(0xc0004c7380, 0x13cac60)
	/usr/local/Cellar/go/1.15.6/libexec/src/testing/testing.go:1123 +0xef
created by testing.(*T).Run
	/usr/local/Cellar/go/1.15.6/libexec/src/testing/testing.go:1168 +0x2b3
FAIL	golang.org/x/text/language	0.138s
FAIL

This is especially concerning since some people pass user-provided data to canonicalize (e.g. the Accept-Lang header).

@seankhliao
Copy link
Member

The function is MustParse which panics intentionally:

MustParse is like Parse, but panics if the given BCP 47 tag cannot be parsed. It simplifies safe initialization of Tag values.

closing as working as intended, please comment/reopen if you disagree

@sethvargo
Copy link
Contributor Author

@seankhliao the function being called is Matcher.Match (interface). That function makes no mention of MustParse, and note that ParseAcceptLanguage successfully parsed the language, but calling matcher.Match causes the panic.

Also note that the panic is not "failed to parse", it's an index out of bounds exception...

@sethvargo
Copy link
Contributor Author

@seankhliao also note there's no direct calls to MustParse or a panic anywhere in those functions. Furthermore, calling MustParse with the language "us-zz" does not panic: https://play.golang.org/p/A3M_rOYffT-

This is a bug, please re-open.

@jeremyfaller
Copy link
Contributor

Reopening for evaluation.

@jeremyfaller jeremyfaller reopened this Jan 21, 2021
@seankhliao
Copy link
Member

I see, but is there a way to trigger it from the exposed api?

@seankhliao seankhliao added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 21, 2021
@seankhliao seankhliao changed the title golang/x/text panic x/text: panic in regionGroupDist Jan 21, 2021
@gopherbot gopherbot added this to the Unreleased milestone Jan 21, 2021
@sethvargo
Copy link
Contributor Author

Yes, that's what led me to file a bug. It's currently impacting a production service: https://github.com/google/exposure-notifications-verification-server/blob/79e397e0f917c78f1d3fe388beeb25e14928095f/internal/i18n/i18n.go#L93

@seankhliao
Copy link
Member

right, reproducer using public api:

package main

import (
	"fmt"

	"golang.org/x/text/language"
)

func main() {
	desired, _, err := language.ParseAcceptLanguage("en-ZZ")
	fmt.Println(desired, err)

	matcher := language.NewMatcher([]language.Tag{language.English})
	t, i, c := matcher.Match(desired...)
	fmt.Println(t, i, c)
}

@seankhliao seankhliao added NeedsFix The path to resolution is known, but the work has not been done. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jan 21, 2021
@jeremyfaller
Copy link
Contributor

CC: @mpvl

This was handed off by the COVID project. Any chance this can get a bump in priority for a fix? They have a workaround using recover(), but my understanding is that it's kinda kludgy.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/305469 mentions this issue: language: fix off-by-one error

@golang golang locked and limited conversation to collaborators Mar 30, 2022
xhit pushed a commit to xhit/text that referenced this issue Oct 10, 2022
Regions are encoded starting from 1. However, one of the
region-related tables assumed 0-based indices. This
caused a crash when used with ZZ, the largest region.

Fixes golang/go#43834

Change-Id: Iaed6b9d2683cd50504e6d33c8a6df8b21dd1687d
Reviewed-on: https://go-review.googlesource.com/c/text/+/305469
Trust: Marcel van Lohuizen <mpvl@golang.org>
Run-TryBot: Marcel van Lohuizen <mpvl@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Seth Vargo <sethvargo@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants