From e5661bcf60477302b66a0cac3ecf3328d9e426e6 Mon Sep 17 00:00:00 2001 From: qmuntal Date: Tue, 8 Oct 2024 11:11:01 +0200 Subject: [PATCH 1/4] prevent RandReader.Read argument from escaping to the heap --- cgo_go124.go | 7 +++++++ rand_test.go | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 cgo_go124.go diff --git a/cgo_go124.go b/cgo_go124.go new file mode 100644 index 00000000..17b572da --- /dev/null +++ b/cgo_go124.go @@ -0,0 +1,7 @@ +//go:build go1.24 && !cmd_go_bootstrap + +package openssl + +// #cgo noescape go_openssl_RAND_bytes +// #cgo nocallback go_openssl_RAND_bytes +import "C" diff --git a/rand_test.go b/rand_test.go index 4735ee75..8fa40192 100644 --- a/rand_test.go +++ b/rand_test.go @@ -1,6 +1,9 @@ package openssl_test import ( + "go/version" + "runtime" + "strings" "testing" "github.com/golang-fips/openssl/v2" @@ -12,3 +15,19 @@ func TestRand(t *testing.T) { t.Fatal(err) } } + +func TestAllocations(t *testing.T) { + n := int(testing.AllocsPerRun(10, func() { + buf := make([]byte, 32) + openssl.RandReader.Read(buf) + sink ^= buf[0] + })) + want := 1 + ver := strings.TrimPrefix(runtime.Version(), "devel ") + if version.Compare(ver, "go1.24") >= 0 { + want = 0 + } + if n > want { + t.Errorf("allocs = %d, want %d", n, want) + } +} From a85271b91219badc598654799f788e82feea0196 Mon Sep 17 00:00:00 2001 From: Quim Muntal Date: Thu, 10 Oct 2024 11:36:25 +0200 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Davis Goodin --- cgo_go124.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cgo_go124.go b/cgo_go124.go index 17b572da..d0287164 100644 --- a/cgo_go124.go +++ b/cgo_go124.go @@ -1,6 +1,16 @@ //go:build go1.24 && !cmd_go_bootstrap package openssl +// The following noescape and nocallback directives are used to prevent the Go +// compiler from allocating function parameters on the heap. See +// https://github.com/golang/go/blob/0733682e5ff4cd294f5eccb31cbe87a543147bc6/src/cmd/cgo/doc.go#L439-L461 +// +// If possible, write a C wrapper function to optimize a call rather than using +// this feature so the optimization will work for all supported Go versions. +// +// This is just a performance optimization. Only add functions that have been +// observed to benefit from these directives, not every function that is merely +// expected to meet the noescape/nocallback criteria. // #cgo noescape go_openssl_RAND_bytes // #cgo nocallback go_openssl_RAND_bytes From 69fa2ee48fc627290c6cb7f817d3f1e3bb2f8127 Mon Sep 17 00:00:00 2001 From: qmuntal Date: Thu, 10 Oct 2024 11:43:53 +0200 Subject: [PATCH 3/4] code review feedback --- openssl_test.go | 13 +++++++++++++ pbkdf2_test.go | 2 -- rand_test.go | 8 +++----- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/openssl_test.go b/openssl_test.go index 07a3c731..182fc7b8 100644 --- a/openssl_test.go +++ b/openssl_test.go @@ -2,14 +2,19 @@ package openssl_test import ( "fmt" + "go/version" "os" "runtime" + "strings" "testing" "time" "github.com/golang-fips/openssl/v2" ) +// sink is used to prevent the compiler from optimizing out the allocations. +var sink uint8 + // getVersion returns the OpenSSL version to use for testing. func getVersion() string { v := os.Getenv("GO_OPENSSL_VERSION_OVERRIDE") @@ -78,3 +83,11 @@ func TestCheckVersion(t *testing.T) { t.Fatalf("FIPS mismatch: want %v, got %v", want, fips) } } + +// compareCurrentVersion compares v with [runtime.Version]. +// See [go/versions.Compare] for information about +// v format and comparison rules. +func compareCurrentVersion(v string) int { + ver := strings.TrimPrefix(runtime.Version(), "devel ") + return version.Compare(ver, v) +} diff --git a/pbkdf2_test.go b/pbkdf2_test.go index 2bea0a25..ff9b8293 100644 --- a/pbkdf2_test.go +++ b/pbkdf2_test.go @@ -166,8 +166,6 @@ func TestWithUnsupportedHash(t *testing.T) { } } -var sink uint8 - func benchmark(b *testing.B, h func() hash.Hash) { password := make([]byte, h().Size()) salt := make([]byte, 8) diff --git a/rand_test.go b/rand_test.go index 8fa40192..6dc956b8 100644 --- a/rand_test.go +++ b/rand_test.go @@ -1,9 +1,6 @@ package openssl_test import ( - "go/version" - "runtime" - "strings" "testing" "github.com/golang-fips/openssl/v2" @@ -23,8 +20,9 @@ func TestAllocations(t *testing.T) { sink ^= buf[0] })) want := 1 - ver := strings.TrimPrefix(runtime.Version(), "devel ") - if version.Compare(ver, "go1.24") >= 0 { + if compareCurrentVersion("go1.24") >= 0 { + // The go1.24 compiler is able to optimize the allocation away. + // See cgo_go124.go for more information. want = 0 } if n > want { From 7be4d35638fd84bf9cf0d4e73112b5f2555f13c1 Mon Sep 17 00:00:00 2001 From: Davis Goodin Date: Thu, 10 Oct 2024 12:02:12 -0700 Subject: [PATCH 4/4] Update cgo_go124.go --- cgo_go124.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cgo_go124.go b/cgo_go124.go index d0287164..933a7518 100644 --- a/cgo_go124.go +++ b/cgo_go124.go @@ -1,6 +1,7 @@ //go:build go1.24 && !cmd_go_bootstrap package openssl + // The following noescape and nocallback directives are used to prevent the Go // compiler from allocating function parameters on the heap. See // https://github.com/golang/go/blob/0733682e5ff4cd294f5eccb31cbe87a543147bc6/src/cmd/cgo/doc.go#L439-L461