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

crypto/cipher: Unnecessary allocations when using boringcrypto's AES-GCM implementation #71139

Open
juergw opened this issue Jan 6, 2025 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance

Comments

@juergw
Copy link

juergw commented Jan 6, 2025

Go version

go1.24-20241213-RC00

Output of go env in your module/workspace:

AR='ar'
CC='clang'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='clang++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/usr/local/google/home/juerg/.cache/go-build'
GODEBUG=''
GOENV='/usr/local/google/home/juerg/.config/go/env'
GOEXE=''
GOEXPERIMENT='fieldtrack,boringcrypto'
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build116742542=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/dev/null'
GOMODCACHE='/usr/local/google/home/juerg/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/usr/local/google/home/juerg/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/google-golang'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/usr/local/google/home/juerg/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/google-golang/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24-20241213-RC00 cl/706019355 +e39e965e0e X:fieldtrack,boringcrypto'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

I ran the following benchmark tests for AES-GCM encryption and decryption:

package aead_test

import (
	"crypto/aes"
	"crypto/cipher"
	"math/rand"
	"testing"
)

const (
	plaintextSize      = 10 * 1024 * 1024
	associatedDataSize = 256
	nonceSize          = 12
)

func GetRandomBytes(n uint32) []byte {
	buf := make([]byte, n)
	_, err := rand.Read(buf)
	if err != nil {
		panic(err)
	}
	return buf
}

func BenchmarkAesGcmEncrypt(b *testing.B) {
	b.ReportAllocs()

	a, err := aes.NewCipher(GetRandomBytes(16))
	if err != nil {
		b.Fatal(err)
	}
	aesGCM, err := cipher.NewGCM(a)
	if err != nil {
		b.Fatal(err)
	}

	plaintext := GetRandomBytes(plaintextSize)
	associatedData := GetRandomBytes(associatedDataSize)
	nonce := GetRandomBytes(nonceSize)

	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		_ = aesGCM.Seal(nil, nonce, plaintext, associatedData)
	}
}

func BenchmarkAesGcmDecrypt(b *testing.B) {
	b.ReportAllocs()

	a, err := aes.NewCipher(GetRandomBytes(16))
	if err != nil {
		b.Fatal(err)
	}
	aesGCM, err := cipher.NewGCM(a)
	if err != nil {
		b.Fatal(err)
	}

	plaintext := GetRandomBytes(plaintextSize)
	associatedData := GetRandomBytes(associatedDataSize)
	nonce := GetRandomBytes(nonceSize)
	ciphertext := aesGCM.Seal(nil, nonce, plaintext, associatedData)

	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		if _, err = aesGCM.Open(nil, nonce, ciphertext, associatedData); err != nil {
			b.Error(err)
		}
	}
}

What did you see happen?

I built the test target and run the benchmark tests with

./aead_test --test.run=NONE --test.bench=. --test.count=1

the output was:

goos: linux
goarch: amd64
pkg: google3/third_party/tink/go/aead/aead_test
cpu: AMD EPYC 7B12
BenchmarkAesGcmEncrypt-8   	     232	   4958494 ns/op	10493980 B/op	       2 allocs/op
BenchmarkAesGcmDecrypt-8   	      58	  26919690 ns/op	52263828 B/op	      48 allocs/op
PASS

The decryption takes 5x longer than the encryption, and does 48 allocations, while the encryption only does 2 allocations.

What did you expect to see?

That the decryption is about as fast as the encryption.

I think the problem is here:

https://github.com/golang/go/blob/master/src/crypto/internal/boring/aes.go#L366

When I replace this loop by this:

		newDstLen := n + len(ciphertext) - gcmTagSize
		if cap(dst) < newDstLen {
			oldDst := dst
			dst = make([]byte, n, newDstLen)
			copy(dst, oldDst)
		}
		dst = dst[:newDstLen]

The issue goes away.

@gabyhelp gabyhelp added the Bug Issues describing a bug in the Go implementation. label Jan 6, 2025
@seankhliao seankhliao added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Bug Issues describing a bug in the Go implementation. labels Jan 6, 2025
@ianlancetaylor
Copy link
Member

CC @golang/security

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

4 participants