x/time/rate: Limiter depends on implementation-defined large float to int conversion

[tomcoupland]

Go version

go version go1.22.1 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE='auto'
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/tomcoupland/Library/Caches/go-build'
GOENV='/Users/tomcoupland/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/tomcoupland/pkg/mod'
GONOPROXY='github.com/monzo'
GONOSUMDB='github.com/monzo'
GOOS='darwin'
GOPATH='/Users/tomcoupland'
GOPRIVATE='github.com/monzo'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/tomcoupland/pkg/mod/golang.org/[email protected]'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/tomcoupland/pkg/mod/golang.org/[email protected]/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.23.1'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/tomcoupland/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/tomcoupland/src/github.com/monzo/wearedev/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/ll/3h483n6j4jbd64x_tjbgwzd40000gn/T/go-build4023913863=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

fmt.Printf("GOARCH: %s\n", runtime.GOARCH)
limiter := rate.NewLimiter(1e-10, 1)

if limiter.Allow() {
  fmt.Println("allow")
} else {
  fmt.Println("not allow")
}
if limiter.Allow() {
  fmt.Println("allow")
} else {
  fmt.Println("not allow")
}

Playground

There is a conversion from float64 to int64 in durationFromTokens, when the final time.Duration is created:

func (limit Limit) durationFromTokens(tokens float64) time.Duration {
	if limit <= 0 {
		return InfDuration
	}
	seconds := tokens / float64(limit)
	return time.Duration(float64(time.Second) * seconds)
}

When limiters are configured with small values (1e-10) this overflows and allows request through that should be blocked. amd64 does not saturate int64's when converting, where as arm64 does. Therefore this code is safe on arm64, but not amd64.

Results of running:

GOARCH = "amd64", see "allow", "allow"
GOARCH = 'arm64', see "allow", "not allow"

This leads to the second "allow" on amd64. I'm preparing a PR with some overflow protection.

What did you see happen?

When GOARCH = "amd64", see "allow", "allow"
Whe GOARCH = 'arm64', see "allow", "not allow"

What did you expect to see?

Regardless of GOARCH: "allow", "not allow"

The first attempt should succeed, due to the burst of 1, but the second should be denied.

[gabyhelp]

Related Issues

• x/time/rate: Limiter depends on implementation-defined float to int conversion #18762
• x/time/rate: function Limiter.AllowN does not work as expected #56145 (closed)
• x/time/rate: Limiter allows more than configured #45245 (closed)
• x/time/rate: Zero limiter allows events #18763
• x/time/rate: function Limiter.AllowN generates much more tokens than expected #56263 (closed)
• x/time/rate: unexpected behaviour with 0 limit #68541 (closed)
• affected/package: x/time/rate #50490 (closed)
• x/time/rate: alternating AllowN before last and after last can create new tokens #56287 (closed)
• x/time/rate: float64 precision loss with edge cases #46579 (closed)
• x/time/rate: Limiter allows more configured with multiple goroutines #65508

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

Is this something that really needs to be fixed? rate = 1e-10 corresponds to a limit of 1 event every 317 years. I think it's more likely that you've misconfigured your program than that being a reasonable limit.

[tomcoupland]

We are developing an automated rate limiting system, that changes the limits dynamically, based on various factors. While we could (and probably will) add logic to prevent these small values escaping, it's a small enough fix to the library to guard against these scenarios, and would prevent any accidents occurring, both for us, and other folks using this rate limiter.

As it stands, the library does nothing to prevent this configuration, and it results in the opposite of what you'd expect.

[ianlancetaylor]

Would you like to send a patch?

[tomcoupland]

I'm just getting it ready, will post it tomorrow. I accept it's pretty far out there, but solving it at this level would shut to door on a class of future bugs.

[tomcoupland]

https://go-review.googlesource.com/c/time/+/641335 - patch submitted

[gopherbot]

Change https://go.dev/cl/641336 mentions this issue: rate: prevent overflows when calculating durationFromTokens