From 113a7a0a0fa092c20672b6e446a769420df79a10 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Thu, 27 Aug 2026 13:44:26 +0200 Subject: [PATCH 01/10] [release-branch.go1.27] crypto/mlkem: fix FIPS 130-3 CAST For #81250 Fixes #81252 Change-Id: I973ead54e02cbc86b488c446c9d581f86a6a6964 Reviewed-on: https://go-review.googlesource.com/c/go/+/822805 Auto-Submit: Filippo Valsorda Reviewed-by: Dmitri Shuralyov Reviewed-by: David Chase LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Roland Shoemaker (cherry picked from commit e656c14b0a259dfd634b7d5905b71f69f387db58) Reviewed-on: https://go-review.googlesource.com/c/go/+/824904 Auto-Submit: Michael Pratt Reviewed-by: Michael Pratt --- .../internal/fips140/mldsa/mldsa_test.go | 4 +- src/crypto/internal/fips140/mlkem/cast.go | 102 +++++++++++------- .../internal/fips140/mlkem/cast_test.go | 34 ++++++ src/crypto/internal/fips140/mlkem/mlkem768.go | 6 ++ 4 files changed, 109 insertions(+), 37 deletions(-) create mode 100644 src/crypto/internal/fips140/mlkem/cast_test.go diff --git a/src/crypto/internal/fips140/mldsa/mldsa_test.go b/src/crypto/internal/fips140/mldsa/mldsa_test.go index 1c0239a30b516d..a2d2279a8e5841 100644 --- a/src/crypto/internal/fips140/mldsa/mldsa_test.go +++ b/src/crypto/internal/fips140/mldsa/mldsa_test.go @@ -346,7 +346,9 @@ func TestCASTRejectionPaths(t *testing.T) { testingOnlyRejectionReason = nil }) - fips140CAST() + if err := fips140CAST(); err != nil { + t.Fatal(err) + } for reason, hit := range reached { if !hit { diff --git a/src/crypto/internal/fips140/mlkem/cast.go b/src/crypto/internal/fips140/mlkem/cast.go index ea089c1b76c0c0..6799540a5daf7f 100644 --- a/src/crypto/internal/fips140/mlkem/cast.go +++ b/src/crypto/internal/fips140/mlkem/cast.go @@ -8,45 +8,75 @@ import ( "bytes" "crypto/internal/fips140" _ "crypto/internal/fips140/check" + "crypto/internal/fips140/sha256" "errors" "sync" ) var fipsSelfTest = sync.OnceFunc(func() { - fips140.CAST("ML-KEM-768", func() error { - var d = &[32]byte{ - 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, - 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, - 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, - 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20, - } - var z = &[32]byte{ - 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28, - 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, 0x30, - 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, - 0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f, 0x40, - } - var m = &[32]byte{ - 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, - 0x49, 0x4a, 0x4b, 0x4c, 0x4d, 0x4e, 0x4f, 0x50, - 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57, 0x58, - 0x59, 0x5a, 0x5b, 0x5c, 0x5d, 0x5e, 0x5f, 0x60, - } - var K = []byte{ - 0x55, 0x01, 0xfc, 0x52, 0x3b, 0x74, 0x5f, 0x41, - 0x76, 0x2a, 0x18, 0x8d, 0xe4, 0x4a, 0x59, 0xb9, - 0x20, 0xf4, 0x30, 0x14, 0x62, 0x04, 0xee, 0x4e, - 0x79, 0x37, 0x32, 0x39, 0x6d, 0xf7, 0xaa, 0x48, - } - dk := &DecapsulationKey768{} - kemKeyGen(dk, d, z) - ek := dk.EncapsulationKey() - var cc [CiphertextSize768]byte - Ke, _ := kemEncaps(&cc, ek, m) - Kd := kemDecaps(dk, &cc) - if !bytes.Equal(Ke, K) || !bytes.Equal(Kd, K) { - return errors.New("unexpected result") - } - return nil - }) + fips140.CAST("ML-KEM-768", fips140CAST) }) + +// fips140CAST covers both the successful and the implicit rejection paths, as +// required by IG 10.3.A, Resolution 14, and as tested by TestCASTRejectionPath. +// It tests only one parameter set as allowed by Note22. It compares dk and not +// ek, because ek is part of dk, as allowed by the same Resolution. It compares +// dk with an expected hash to avoid embedding several kilobytes of test vectors +// in every binary, as allowed by GeneralNote7. +func fips140CAST() error { + var d = &[32]byte{ + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, + 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, + 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, + 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20, + } + var z = &[32]byte{ + 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28, + 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, 0x30, + 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, + 0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f, 0x40, + } + var dkHash = []byte{ + 0xa1, 0x32, 0x4a, 0x71, 0xf5, 0x7e, 0x26, 0x85, + 0x37, 0x7e, 0x4f, 0x7d, 0xe2, 0xd1, 0xf0, 0x58, + 0x53, 0x8e, 0x08, 0x7e, 0xe3, 0x9b, 0x48, 0x2b, + 0x3d, 0xbf, 0x96, 0xf9, 0xf7, 0x69, 0xc7, 0x85, + } + var m = &[32]byte{ + 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, + 0x49, 0x4a, 0x4b, 0x4c, 0x4d, 0x4e, 0x4f, 0x50, + 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57, 0x58, + 0x59, 0x5a, 0x5b, 0x5c, 0x5d, 0x5e, 0x5f, 0x60, + } + var K = []byte{ + 0x55, 0x01, 0xfc, 0x52, 0x3b, 0x74, 0x5f, 0x41, + 0x76, 0x2a, 0x18, 0x8d, 0xe4, 0x4a, 0x59, 0xb9, + 0x20, 0xf4, 0x30, 0x14, 0x62, 0x04, 0xee, 0x4e, + 0x79, 0x37, 0x32, 0x39, 0x6d, 0xf7, 0xaa, 0x48, + } + var Kr = []byte{ + 0x01, 0xa1, 0x03, 0x3a, 0x2e, 0x68, 0x33, 0x80, + 0x7f, 0xd3, 0x91, 0x80, 0xbe, 0x3b, 0x4f, 0x0a, + 0x1f, 0x6f, 0x50, 0x51, 0x67, 0x56, 0x9d, 0x0e, + 0x5e, 0x5e, 0x1f, 0xdb, 0x52, 0xa9, 0x10, 0xec, + } + dk := &DecapsulationKey768{} + kemKeyGen(dk, d, z) + ek := dk.EncapsulationKey() + H := sha256.New() + H.Write(TestingOnlyExpandedBytes768(dk)) + if !bytes.Equal(H.Sum(nil), dkHash) { + return errors.New("unexpected private key hash") + } + var cc [CiphertextSize768]byte + Kd := kemDecaps(dk, &cc) + if !bytes.Equal(Kd, Kr) { + return errors.New("unexpected rejection result") + } + Ke, _ := kemEncaps(&cc, ek, m) + Kd = kemDecaps(dk, &cc) + if !bytes.Equal(Ke, K) || !bytes.Equal(Kd, K) { + return errors.New("unexpected result") + } + return nil +} diff --git a/src/crypto/internal/fips140/mlkem/cast_test.go b/src/crypto/internal/fips140/mlkem/cast_test.go new file mode 100644 index 00000000000000..b28b6c01e860d6 --- /dev/null +++ b/src/crypto/internal/fips140/mlkem/cast_test.go @@ -0,0 +1,34 @@ +// Copyright 2026 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package mlkem + +import "testing" + +func TestCASTRejectionPaths(t *testing.T) { + var rejected, notRejected bool + testingOnlyRejectionOutcome = func(compare int) { + if compare == 1 { + t.Log("non-rejection path hit") + notRejected = true + } else { + t.Log("rejection path hit") + rejected = true + } + } + t.Cleanup(func() { + testingOnlyRejectionOutcome = nil + }) + + if err := fips140CAST(); err != nil { + t.Fatal(err) + } + + if !rejected { + t.Error("rejection path not hit") + } + if !notRejected { + t.Error("non-rejection path not hit") + } +} diff --git a/src/crypto/internal/fips140/mlkem/mlkem768.go b/src/crypto/internal/fips140/mlkem/mlkem768.go index c4c3a9deaf4d7d..9c5e45e535ab1f 100644 --- a/src/crypto/internal/fips140/mlkem/mlkem768.go +++ b/src/crypto/internal/fips140/mlkem/mlkem768.go @@ -469,6 +469,8 @@ func (dk *DecapsulationKey768) Decapsulate(ciphertext []byte) (sharedKey []byte, return kemDecaps(dk, c), nil } +var testingOnlyRejectionOutcome func(int) + // kemDecaps produces a shared key from a ciphertext. // // It implements ML-KEM.Decaps_internal according to FIPS 203, Algorithm 18. @@ -488,6 +490,10 @@ func kemDecaps(dk *DecapsulationKey768, c *[CiphertextSize768]byte) (K []byte) { var cc [CiphertextSize768]byte c1 := pkeEncrypt(&cc, &dk.encryptionKey, (*[32]byte)(m), r) + if testingOnlyRejectionOutcome != nil { + testingOnlyRejectionOutcome(subtle.ConstantTimeCompare(c[:], c1)) + } + subtle.ConstantTimeCopy(subtle.ConstantTimeCompare(c[:], c1), Kout, Kprime) return Kout } From 33c04bd7da81bd56ffaf05bb1e9a9820ddc8ab3e Mon Sep 17 00:00:00 2001 From: Rhys Hiltner Date: Thu, 20 Aug 2026 19:30:49 -0700 Subject: [PATCH 02/10] [release-branch.go1.27] runtime: disable mutex profile during async preemption on windows Async preemption on Windows includes some synchronous components: one thread pauses another and inspects and operates on its state. If the paused thread is holding locks, the thread doing the preemption won't be able to acquire them, just as if it were a signal handler running on the paused thread. Mutex profiling results in acquiring a mutex within runtime.unlock, if the code within unlock2 determines that the current M doesn't hold any other locks. Usually signal handlers aren't allowed to acquire any locks at all, but Windows' preemptM does. Communicate the restriction to unlock2 with the same mechanism we use when a gscan bit is held: with a call to acquireLockRankAndM. Fixes #81145 Change-Id: I722bb9ce8e15e01ec70bb0efc1bec4eca823cfa6 Reviewed-on: https://go-review.googlesource.com/c/go/+/818940 Auto-Submit: Rhys Hiltner Reviewed-by: Dmitri Shuralyov Reviewed-by: David Chase LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Zxilly Chou Reviewed-by: Quim Muntal Reviewed-by: Russ Cox (cherry picked from commit 3d727ce0699ffbb4e5c58d9e1bb5638e16cb24f9) Reviewed-on: https://go-review.googlesource.com/c/go/+/824104 Reviewed-by: Keith Randall Reviewed-by: Keith Randall --- src/runtime/os_windows.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/runtime/os_windows.go b/src/runtime/os_windows.go index 42af4200547b64..73e2b213f3f030 100644 --- a/src/runtime/os_windows.go +++ b/src/runtime/os_windows.go @@ -1176,6 +1176,23 @@ const preemptMSupported = true // suspending each other. var suspendLock mutex +// threadPausedLockRank warns the runtime against acquiring new locks on this +// thread while it has another thread paused. The holder of this lock rank is +// effectively a signal handler for a paused thread, which may be holding +// arbitrary locks. +// +// The warning takes two concrete forms: First, unlock2 will see that this +// thread is holding other locks, so won't try to flush the mutex contention +// buffer (which can involve acquiring another lock). Second, builds with +// GOEXPERIMENT=staticlockranking (currently broken, https://go.dev/issue/76058) +// will report a rank violation. +// +// suspendLock serializes the transition into "suspended", but it's fine for +// multiple threads to be in the suspended state concurrently, with each of +// their sponsors independently using threadPausedLockRank to discourage +// themselves from acquiring new locks. +const threadPausedLockRank = lockRankLeafRank + func preemptM(mp *m) { if mp == getg().m { throw("self-preempt") @@ -1218,6 +1235,9 @@ func preemptM(mp *m) { // actually suspended. lock(&suspendLock) + // Signal handler rules are in effect; don't acquire new locks. + acquireLockRankAndM(threadPausedLockRank) + // Suspend the thread. if int32(stdcall(_SuspendThread, thread)) == -1 { unlock(&suspendLock) @@ -1226,6 +1246,7 @@ func preemptM(mp *m) { // The thread no longer exists. This shouldn't be // possible, but just acknowledge the request. mp.preemptGen.Add(1) + releaseLockRankAndM(threadPausedLockRank) return } @@ -1260,6 +1281,8 @@ func preemptM(mp *m) { stdcall(_ResumeThread, thread) stdcall(_CloseHandle, thread) + + releaseLockRankAndM(threadPausedLockRank) } // osPreemptExtEnter is called before entering external code that may From 2ee6421c51553e7164590445f96b123a858c1f4a Mon Sep 17 00:00:00 2001 From: Richard Weinberger Date: Sun, 23 Aug 2026 20:53:38 +0000 Subject: [PATCH 03/10] [release-branch.go1.27] runtime: netpoll: fix 32-bit little-endian eventfd type confusion netpoll must distinguish the netpollBreak eventfd from a socket fd. It does so by comparing ev.Data against &netpollEventFd: if they are equal the event is for the eventfd, otherwise it is for a socket and ev.Data holds a tagged pointer made up of a *pollDesc and an fdseq. On 64-bit systems this works as expected, but on 32-bit little-endian systems it can lead to type confusion. netpollinit stores the address of netpollEventFd as a uintptr: *(**uintptr)(unsafe.Pointer(&ev.Data)) = &netpollEventFd so for the eventfd only the lower half of ev.Data is written: ev.Data[0:4] holds &netpollEventFd and ev.Data[4:8] is left untouched. netpollopen, in contrast, stores a full tagged pointer, so that ev.Data[0:4] holds fdseq and ev.Data[4:8] holds the *pollDesc. Because a uintptr is only 4 bytes on 32-bit systems, the comparison in netpoll: if *(**uintptr)(unsafe.Pointer(&ev.Data)) == &netpollEventFd { tests only ev.Data[0:4], so for a socket it effectively compares fdseq against the address of netpollEventFd. In the vast majority of cases the two differ and the fd types are distinguished correctly. However, in a program that runs for a long time or opens many sockets, fdseq can eventually grow large enough to equal &netpollEventFd. When that happens netpoll mistakes a socket for the eventfd and the runtime aborts with a fatal error such as: runtime: netpoll: eventfd ready for 5 fatal error: runtime: netpoll: eventfd ready for something unexpected Fix this by using the tagged-pointer encoding for both cases and detecting the eventfd by its nil *pollDesc. Updates #72900 Fixes #81226 Change-Id: I9779dc038f437eeb23fbd1b7057eee5f5951ac13 GitHub-Last-Rev: 521495e95855f696f4d3c1780758f0545e76f44f GitHub-Pull-Request: golang/go#81037 Reviewed-on: https://go-review.googlesource.com/c/go/+/819800 Reviewed-by: Ian Lance Taylor Reviewed-by: David Chase LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Cherry Mui (cherry picked from commit b9ba134da610a22c8368df37457aa0eb05579eb7) Reviewed-on: https://go-review.googlesource.com/c/go/+/824304 Auto-Submit: Michael Pratt Reviewed-by: Alan Donovan Reviewed-by: Michael Pratt --- src/runtime/netpoll_epoll.go | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/runtime/netpoll_epoll.go b/src/runtime/netpoll_epoll.go index e658cc3d89c666..d6d00e67e917ff 100644 --- a/src/runtime/netpoll_epoll.go +++ b/src/runtime/netpoll_epoll.go @@ -18,6 +18,21 @@ var ( netpollWakeSig atomic.Uint32 // used to avoid duplicate calls of netpollBreak ) +// netpollPackData stores pd and its fdseq in the epoll data field of ev as +// a taggedPointer. A nil pd marks the netpollBreak eventfd: netpollopen always +// registers a non-nil *pollDesc, so netpollUnpackData can tell the two apart +// by the pointer alone, on any word size or endianness. +func netpollPackData(ev *linux.EpollEvent, pd *pollDesc, fdseq uintptr) { + *(*taggedPointer)(unsafe.Pointer(&ev.Data)) = taggedPointerPack(unsafe.Pointer(pd), fdseq) +} + +// netpollUnpackData returns the pollDesc and fdseq stored by netpollPackData. +// pd is nil for the netpollBreak eventfd. +func netpollUnpackData(ev *linux.EpollEvent) (pd *pollDesc, fdseq uintptr) { + tp := *(*taggedPointer)(unsafe.Pointer(&ev.Data)) + return (*pollDesc)(tp.pointer()), tp.tag() +} + func netpollinit() { var errno uintptr epfd, errno = linux.EpollCreate1(linux.EPOLL_CLOEXEC) @@ -33,7 +48,7 @@ func netpollinit() { ev := linux.EpollEvent{ Events: linux.EPOLLIN, } - *(**uintptr)(unsafe.Pointer(&ev.Data)) = &netpollEventFd + netpollPackData(&ev, nil, 0) errno = linux.EpollCtl(epfd, linux.EPOLL_CTL_ADD, efd, &ev) if errno != 0 { println("runtime: epollctl failed with", errno) @@ -49,8 +64,7 @@ func netpollIsPollDescriptor(fd uintptr) bool { func netpollopen(fd uintptr, pd *pollDesc) uintptr { var ev linux.EpollEvent ev.Events = linux.EPOLLIN | linux.EPOLLOUT | linux.EPOLLRDHUP | linux.EPOLLET - tp := taggedPointerPack(unsafe.Pointer(pd), pd.fdseq.Load()) - *(*taggedPointer)(unsafe.Pointer(&ev.Data)) = tp + netpollPackData(&ev, pd, pd.fdseq.Load()) return linux.EpollCtl(epfd, linux.EPOLL_CTL_ADD, int32(fd), &ev) } @@ -137,7 +151,9 @@ retry: continue } - if *(**uintptr)(unsafe.Pointer(&ev.Data)) == &netpollEventFd { + pd, tag := netpollUnpackData(&ev) + // pd == nil denotes a netpollBreak eventfd. + if pd == nil { if ev.Events != linux.EPOLLIN { println("runtime: netpoll: eventfd ready for", ev.Events) throw("runtime: netpoll: eventfd ready for something unexpected") @@ -163,9 +179,6 @@ retry: mode += 'w' } if mode != 0 { - tp := *(*taggedPointer)(unsafe.Pointer(&ev.Data)) - pd := (*pollDesc)(tp.pointer()) - tag := tp.tag() if pd.fdseq.Load() == tag { pd.setEventErr(ev.Events == linux.EPOLLERR, tag) delta += netpollready(&toRun, pd, mode) From 55e42d430793133e3564841c48be7df11db1b805 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 1 Sep 2026 02:02:25 +0000 Subject: [PATCH 04/10] [tailscale-cherrypick] net/http/internal/http2: don't use a per-Server error channel pool The move of HTTP/2 into std made errChanPool a field on http2.Server so that pooled channels wouldn't be reused across synctest bubbles. But that regressed memory for servers using the ServeConn-per-conn pattern (HTTP/2 over hijacked or tunneled conns): every sync.Pool re-pins after each GC, allocating a GOMAXPROCS-sized poolLocal array (4 KB at GOMAXPROCS=32) per pool, and a per-conn pool provides no reuse anyway. On a production proxy with ~400k conns, the poolLocal arrays accounted for ~2 GB, ~10% of heap. Instead, go back to a single global pool and skip pooling entirely when the current goroutine is in a synctest bubble. Updates tailscale/go#174 Change-Id: I8a8df547507b185b7943ab8d227da8262acf8a49 Reviewed-on: https://go-review.googlesource.com/c/go/+/825424 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Damien Neil Reviewed-by: Nicholas Husin Reviewed-by: Nicholas Husin --- src/net/http/internal/http2/server.go | 44 +++++++++++++-------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/net/http/internal/http2/server.go b/src/net/http/internal/http2/server.go index e7ed546c6526ab..8694027a0fe88c 100644 --- a/src/net/http/internal/http2/server.go +++ b/src/net/http/internal/http2/server.go @@ -33,6 +33,7 @@ import ( "crypto/tls" "errors" "fmt" + "internal/synctest" "io" "log" "math" @@ -103,10 +104,6 @@ var ( type Server struct { mu sync.Mutex activeConns map[*serverConn]struct{} - - // Pool of error channels. This is per-Server rather than global - // because channels can't be reused across synctest bubbles. - errChanPool sync.Pool } func (s *Server) registerConn(sc *serverConn) { @@ -138,30 +135,33 @@ func (s *Server) startGracefulShutdown() { s.mu.Unlock() } -// Global error channel pool used for uninitialized Servers. -// We use a per-Server pool when possible to avoid using channels across synctest bubbles. +// errChanPool is a pool of reusable channels for reporting the result +// of a blocking frame write. +// +// The pool is not used inside synctest bubbles, since a channel created +// in one bubble can't be used from another bubble or from outside a +// bubble, and sync.Pool is not bubble-aware. var errChanPool = sync.Pool{ New: func() any { return make(chan error, 1) }, } -func (s *Server) getErrChan() chan error { - if s == nil { - return errChanPool.Get().(chan error) // Server used without calling ConfigureServer +func getErrChan() chan error { + if synctest.IsInBubble() { + // Channels can't be shared across synctest bubbles. + // Skip the pool; allocation cost is irrelevant in tests. + return make(chan error, 1) } - return s.errChanPool.Get().(chan error) + return errChanPool.Get().(chan error) } -func (s *Server) putErrChan(ch chan error) { - if s == nil { - errChanPool.Put(ch) // Server used without calling ConfigureServer - return +func putErrChan(ch chan error) { + if !synctest.IsInBubble() { + errChanPool.Put(ch) } - s.errChanPool.Put(ch) } func (s *Server) Configure(conf ServerConfig, tcfg *tls.Config) error { s.activeConns = make(map[*serverConn]struct{}) - s.errChanPool = sync.Pool{New: func() any { return make(chan error, 1) }} if tcfg.CipherSuites != nil && tcfg.MinVersion < tls.VersionTLS13 { // If they already provided a TLS 1.0–1.2 CipherSuite list, return an @@ -1009,7 +1009,7 @@ var writeDataPool = sync.Pool{ // writeDataFromHandler writes DATA response frames from a handler on // the given stream. func (sc *serverConn) writeDataFromHandler(stream *stream, data []byte, endStream bool) error { - ch := sc.srv.getErrChan() + ch := getErrChan() writeArg := writeDataPool.Get().(*writeData) *writeArg = writeData{stream.id, data, endStream} err := sc.writeFrameFromHandler(FrameWriteRequest{ @@ -1041,7 +1041,7 @@ func (sc *serverConn) writeDataFromHandler(stream *stream, data []byte, endStrea return errStreamClosed } } - sc.srv.putErrChan(ch) + putErrChan(ch) if frameWriteDone { writeDataPool.Put(writeArg) } @@ -2343,7 +2343,7 @@ func (sc *serverConn) writeHeaders(st *stream, headerData *writeResHeaders) erro // waiting for this frame to be written, so an http.Flush mid-handler // writes out the correct value of keys, before a handler later potentially // mutates it. - errc = sc.srv.getErrChan() + errc = getErrChan() } if err := sc.writeFrameFromHandler(FrameWriteRequest{ write: headerData, @@ -2355,7 +2355,7 @@ func (sc *serverConn) writeHeaders(st *stream, headerData *writeResHeaders) erro if errc != nil { select { case err := <-errc: - sc.srv.putErrChan(errc) + putErrChan(errc) return err case <-sc.doneServing: return errClientDisconnected @@ -3061,7 +3061,7 @@ func (w *responseWriter) Push(target, method string, header Header) error { method: method, url: u, header: cloneHeader(header), - done: sc.srv.getErrChan(), + done: getErrChan(), } select { @@ -3078,7 +3078,7 @@ func (w *responseWriter) Push(target, method string, header Header) error { case <-st.cw: return errStreamClosed case err := <-msg.done: - sc.srv.putErrChan(msg.done) + putErrChan(msg.done) return err } } From 94d0670ac57a3dae8aa1fad855dcf3da2861cba7 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 3 Sep 2026 04:21:21 +0000 Subject: [PATCH 05/10] [tailscale-cherrypick] net/http/internal/http2: don't shrink HPACK decoder table before SETTINGS ack A server configured with a MaxDecoderHeaderTableSize below the initial 4096 bytes (RFC 7540, section 6.5.2) applied it to its HPACK decoder at connection start. But the server doesn't know when the client has received and applied a SETTINGS value until the client acknowledges the SETTINGS frame (RFC 7540, section 6.5.3), and the client's encoder signals a table size reduction with a dynamic table size update at the beginning of the first header block following the settings acknowledgment (RFC 7541, section 4.2). Until then, the client may keep using the initial 4096-byte table. A spec-compliant client (including Go's own http2 client) that sent a header block referencing a dynamic table entry in the first RTT was rejected with a connection-fatal COMPRESSION_ERROR. Instead, start the decoder at the initial table size and apply the configured smaller size once the client acks our SETTINGS. All header blocks after the ack were encoded by a client that has processed the setting, and a compliant encoder begins its next header block with a dynamic table size update. Memory during the window is bounded by the same 4096 bytes as today's default. Updates tailscale/go#180 Change-Id: Idecd25a1e0473c3cff314922ddc97ace14cb2e5e Reviewed-on: https://go-review.googlesource.com/c/go/+/827024 Reviewed-by: Nicholas Husin LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Damien Neil Reviewed-by: Nicholas Husin Auto-Submit: Brad Fitzpatrick --- src/net/http/internal/http2/server.go | 26 ++++++- src/net/http/internal/http2/server_test.go | 82 ++++++++++++++++++++++ 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/src/net/http/internal/http2/server.go b/src/net/http/internal/http2/server.go index 8694027a0fe88c..bd2533a7c36e66 100644 --- a/src/net/http/internal/http2/server.go +++ b/src/net/http/internal/http2/server.go @@ -321,7 +321,19 @@ func (s *Server) serveConn(c net.Conn, opts *ServeConnOpts, newf func(*serverCon if conf.CountError != nil { fr.countError = conf.CountError } - fr.ReadMetaHeaders = hpack.NewDecoder(uint32(conf.MaxDecoderHeaderTableSize), nil) + // A decoder table size below the initial 4096 (RFC 7540, Section 6.5.2) + // can't be applied immediately: the client may keep using the initial + // size until it processes our SETTINGS frame (RFC 7540, Section 6.5.3), + // and it signals the reduction with a dynamic table size update at the + // beginning of the first header block following the settings + // acknowledgment (RFC 7541, Section 4.2). Start at the initial size and + // lower it when the client acknowledges our SETTINGS. See processSettings. + decoderTableSize := uint32(conf.MaxDecoderHeaderTableSize) + if decoderTableSize < initialHeaderTableSize { + sc.pendingDecoderTableSize = decoderTableSize + decoderTableSize = initialHeaderTableSize + } + fr.ReadMetaHeaders = hpack.NewDecoder(decoderTableSize, nil) fr.MaxHeaderListSize = sc.maxHeaderListSize() fr.MaxHeaderValueCount = sc.hs.MaxHeaderValueCount() fr.SetMaxReadFrameSize(uint32(conf.MaxReadFrameSize)) @@ -443,6 +455,7 @@ type serverConn struct { sawFirstSettings bool // got the initial SETTINGS frame after the preface needToSendSettingsAck bool unackedSettings int // how many SETTINGS have we sent without ACKs? + pendingDecoderTableSize uint32 // if non-zero, HPACK decoder table size to apply on SETTINGS ack queuedControlFrames int // control frames in the writeSched queue clientMaxStreams uint32 // SETTINGS_MAX_CONCURRENT_STREAMS from client (our PUSH_PROMISE limit) advMaxStreams uint32 // our SETTINGS_MAX_CONCURRENT_STREAMS advertised the client @@ -1614,6 +1627,17 @@ func (sc *serverConn) processSettings(f *SettingsFrame) error { // hang up on them anyway. return sc.countError("ack_mystery", ConnectionError(ErrCodeProtocol)) } + if sc.pendingDecoderTableSize != 0 { + // The client has acknowledged our SETTINGS, so all header + // blocks it sends from now on were encoded with knowledge + // of our lower HEADER_TABLE_SIZE. It is now safe to apply + // the configured size to the decoder. The read goroutine + // is parked until readMore is called, so mutating the + // decoder here is race-free. + sc.framer.ReadMetaHeaders.SetAllowedMaxDynamicTableSize(sc.pendingDecoderTableSize) + sc.framer.ReadMetaHeaders.SetMaxDynamicTableSize(sc.pendingDecoderTableSize) + sc.pendingDecoderTableSize = 0 + } return nil } if f.NumSettings() > 100 || f.HasDuplicates() { diff --git a/src/net/http/internal/http2/server_test.go b/src/net/http/internal/http2/server_test.go index 9e1374b6e05d20..f15ad9e666cb10 100644 --- a/src/net/http/internal/http2/server_test.go +++ b/src/net/http/internal/http2/server_test.go @@ -2824,6 +2824,88 @@ func testServer_MaxDecoderHeaderTableSize(t *testing.T) { } } +func TestServer_MaxDecoderHeaderTableSize_DeferredUntilAck(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + testServer_MaxDecoderHeaderTableSizeDeferred(t, true) + }) +} + +func TestServer_MaxDecoderHeaderTableSize_EnforcedAfterAck(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + testServer_MaxDecoderHeaderTableSizeDeferred(t, false) + }) +} + +// testServer_MaxDecoderHeaderTableSizeDeferred tests that a server +// configured with a decoder header table size below the protocol-initial +// 4096 bytes does not apply it until the client acknowledges the server's +// SETTINGS frame. Until then, the client's encoder may legitimately +// reference dynamic table entries under the initial 4096-byte table size +// (RFC 7540, Section 6.5.3; RFC 7541, Section 4.2). +func testServer_MaxDecoderHeaderTableSizeDeferred(t *testing.T, sizeUpdateAfterAck bool) { + st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {}, func(h2 *http.HTTP2Config) { + h2.MaxDecoderHeaderTableSize = 1 + }) + defer st.Close() + + st.writePreface() + st.writeSettings() + st.sync() + readFrame[*SettingsFrame](t, st) + + // The server's initial WINDOW_UPDATE and its ack of our SETTINGS + // can arrive in either order. + st.wantUnorderedFrames( + func(f *SettingsFrame) bool { + if !f.IsAck() { + t.Fatalf("got second non-ack SETTINGS frame") + } + return true + }, + func(f *WindowUpdateFrame) bool { + return true + }, + ) + + // Send two requests whose header blocks use the dynamic table, before + // acknowledging the server's SETTINGS. The second request references + // entries added by the first. Our encoder is still permitted to use + // the initial 4096-byte table size, so both must succeed. + for _, id := range []uint32{1, 3} { + st.writeHeaders(HeadersFrameParam{ + StreamID: id, + BlockFragment: st.encodeHeader("x-custom-header", "long-enough-value-to-index"), + EndStream: true, + EndHeaders: true, + }) + st.wantHeaders(wantHeader{streamID: id, endStream: true}) + } + + // Acknowledge the server's SETTINGS. Header blocks we send from here + // on must respect the reduced table size. + st.writeSettingsAck() + st.sync() + + if sizeUpdateAfterAck { + // A compliant encoder begins its next header block with a + // dynamic table size update. The request must succeed. + st.hpackEnc.SetMaxDynamicTableSize(1) + } + st.writeHeaders(HeadersFrameParam{ + StreamID: 5, + BlockFragment: st.encodeHeader("x-custom-header", "long-enough-value-to-index"), + EndStream: true, + EndHeaders: true, + }) + if sizeUpdateAfterAck { + st.wantHeaders(wantHeader{streamID: 5, endStream: true}) + } else { + // A header block that keeps referencing the now-evicted + // 4096-byte table without a size update must be rejected. + st.wantGoAway(5, ErrCodeCompression) + } +} + func TestServer_MaxEncoderHeaderTableSize(t *testing.T) { synctest.Test(t, testServer_MaxEncoderHeaderTableSize) } From f1a063e4ed08f3bdc4eec9b636f6091eff90fc36 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 2 Sep 2026 02:37:57 +0000 Subject: [PATCH 06/10] [tailscale-cherrypick] crypto/tls: don't pin record-sized buffers while blocked in Read A Conn's rawInput buffer grows to the maximum record size (~17 kB after receiving full-sized records) and, being a bytes.Buffer, never shrinks. A connection blocked in Read waiting for a new record, often for minutes or hours on long-polling or mostly idle connections, pinned that memory the whole time. Servers with millions of open TLS connections strand gigabytes of heap in buffers holding no data. The hand buffer similarly retained the peer's largest handshake flight for the life of the connection. Instead, return an empty grown rawInput buffer to a sync.Pool before blocking to wait for a new record, mirroring outBufPool on the write side. The record header is read into a small per-connection buffer, and readFromUntil switches back to a pooled record-sized buffer only once the header arrives and the payload length is known. The hand buffer is likewise pooled: it is returned once the handshake completes and after buffered post-handshake messages are consumed. Measured with 1000 idle TLS 1.3 server connections that had each received 16 kB records before blocking in a 4-byte Read (heap bytes per connection, runtime.MemProfileRate=1): HeapAlloc/conn: 23020 B -> 4468 B HeapInuse/conn: 25509 B -> 5726 B The remainder is mostly the AES-GCM cipher states (~1.8 kB), the Conn struct itself (~1 kB), and the small header buffer (~0.6 kB). Throughput is unchanged within noise (geomean +0.4%): benchstat of -bench=Throughput -count=6: no change in 17 of 28 cases, worst case +1.2%, best case -1.8%. Pooling the hand buffer also drops an allocation and ~1% of bytes per server handshake (geomean of -bench=HandshakeServer -benchmem). Fixes #47672 Updates #81348 Updates tailscale/go#174 Change-Id: Idb6f9361d888977a2d655fb2d57da0fa61f61ca5 Reviewed-on: https://go-review.googlesource.com/c/go/+/827524 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Roland Shoemaker Reviewed-by: Daniel McCarney Reviewed-by: Michael Pratt --- src/crypto/tls/conn.go | 137 ++++++++++++++++++++--- src/crypto/tls/handshake_server_tls13.go | 2 +- src/crypto/tls/quic.go | 11 +- 3 files changed, 131 insertions(+), 19 deletions(-) diff --git a/src/crypto/tls/conn.go b/src/crypto/tls/conn.go index c6d30841b6c2cf..d48316017806ae 100644 --- a/src/crypto/tls/conn.go +++ b/src/crypto/tls/conn.go @@ -99,12 +99,27 @@ type Conn struct { clientProtocol string // input/output - in, out halfConn - rawInput bytes.Buffer // raw input, starting with a record header - input bytes.Reader // application data waiting to be read, from rawInput.Next - hand bytes.Buffer // handshake data waiting to be read - buffering bool // whether records are buffered in sendBuf - sendBuf []byte // a buffer of records waiting to be sent + in, out halfConn + // rawInput holds raw input, starting with a record header. + // It is nil when no input is buffered, in which case the buffer has + // been returned to rawInputPool so that connections idle in Read do + // not pin a record-sized buffer. It is lazily repopulated from the + // pool by readFromUntil. + rawInput *bytes.Buffer + // smallInput is a small buffer that serves as rawInput while + // waiting for a record header after rawInput has been returned to + // rawInputPool. It is lazily allocated by readFromUntil and then + // kept for the life of the connection. + smallInput *bytes.Buffer + // input holds application data waiting to be read, from rawInput.Next. + input bytes.Reader + // hand holds handshake data waiting to be read. + // It is nil when no handshake data is buffered, in which case the + // buffer has been returned to handPool. Use handBuf and handLen to + // access it. + hand *bytes.Buffer + buffering bool // whether records are buffered in sendBuf + sendBuf []byte // a buffer of records waiting to be sent // bytesSent counts the bytes of application data sent. // packetsSent counts packets. @@ -580,7 +595,9 @@ func (e RecordHeaderError) Error() string { return "tls: " + e.Msg } func (c *Conn) newRecordHeaderError(conn net.Conn, msg string) (err RecordHeaderError) { err.Msg = msg err.Conn = conn - copy(err.RecordHeader[:], c.rawInput.Bytes()) + if c.rawInput != nil { + copy(err.RecordHeader[:], c.rawInput.Bytes()) + } return err } @@ -622,6 +639,19 @@ func (c *Conn) readRecordOrCCS(expectChangeCipherSpec bool) error { return c.in.setErrorLocked(errors.New("tls: internal error: attempted to read record with QUIC transport")) } + // If rawInput is empty, we are about to block in a Read on the + // underlying connection waiting for the next record, possibly for a + // long time. A previous record may have grown rawInput to the maximum + // record size; don't pin that memory while idle. Return the buffer to + // the pool, and let readFromUntil read the header into a small buffer + // and switch back to a pooled record-sized buffer only once the + // payload length is known. + if c.rawInput != nil && c.rawInput.Len() == 0 && c.rawInput != c.smallInput && c.rawInput.Cap() > maxIdleInputCap { + c.rawInput.Reset() + rawInputPool.Put(c.rawInput) + c.rawInput = nil + } + // Read header, payload. if err := c.readFromUntil(c.conn, recordHeaderLen); err != nil { // RFC 8446, Section 6.1 suggests that EOF without an alertCloseNotify @@ -702,7 +732,7 @@ func (c *Conn) readRecordOrCCS(expectChangeCipherSpec bool) error { } // Handshake messages MUST NOT be interleaved with other record types in TLS 1.3. - if c.vers == VersionTLS13 && typ != recordTypeHandshake && c.hand.Len() > 0 { + if c.vers == VersionTLS13 && typ != recordTypeHandshake && c.handLen() > 0 { return c.in.setErrorLocked(c.sendAlert(alertUnexpectedMessage)) } @@ -747,7 +777,7 @@ func (c *Conn) readRecordOrCCS(expectChangeCipherSpec bool) error { return c.in.setErrorLocked(c.sendAlert(alertDecodeError)) } // Handshake messages are not allowed to fragment across the CCS. - if c.hand.Len() > 0 { + if c.handLen() > 0 { return c.in.setErrorLocked(c.sendAlert(alertUnexpectedMessage)) } // In TLS 1.3, change_cipher_spec records are ignored until the @@ -783,7 +813,7 @@ func (c *Conn) readRecordOrCCS(expectChangeCipherSpec bool) error { if len(data) == 0 || expectChangeCipherSpec { return c.in.setErrorLocked(c.sendAlert(alertUnexpectedMessage)) } - c.hand.Write(data) + c.handBuf().Write(data) } return nil @@ -800,13 +830,85 @@ func (c *Conn) retryReadRecord(expectChangeCipherSpec bool) error { return c.readRecordOrCCS(expectChangeCipherSpec) } +// rawInputPool pools the record-sized buffers that back Conn.rawInput +// while records are being received. A connection returns its buffer to +// the pool before blocking to wait for a new record, often for a long +// time, so that idle connections do not each pin a record-sized buffer. +// Only buffers with capacity above maxIdleInputCap are pooled; smaller +// buffers stay attached to their connection. +var rawInputPool = sync.Pool{New: func() any { return new(bytes.Buffer) }} + +// maxIdleInputCap is the largest rawInput capacity that a connection +// keeps while waiting for a new record to arrive. It is large enough to +// hold a record header and small records, so that only connections +// receiving larger records pay for the pooled buffer switch below. +const maxIdleInputCap = 1024 + +// handPool pools the buffers that back Conn.hand, which typically grow +// to hold the peer's largest flight of handshake messages. A connection +// returns its buffer to the pool once the handshake completes and after +// buffered post-handshake messages have been consumed, so that +// established connections do not pin it. +var handPool = sync.Pool{New: func() any { return new(bytes.Buffer) }} + +// handBuf returns c.hand for writing, getting a buffer from handPool if +// c.hand is nil. +func (c *Conn) handBuf() *bytes.Buffer { + if c.hand == nil { + c.hand = handPool.Get().(*bytes.Buffer) + } + return c.hand +} + +// handLen returns the number of buffered handshake bytes. +func (c *Conn) handLen() int { + if c.hand == nil { + return 0 + } + return c.hand.Len() +} + +// releaseHand returns c.hand to handPool if it is empty. +func (c *Conn) releaseHand() { + if c.hand != nil && c.hand.Len() == 0 { + c.hand.Reset() + handPool.Put(c.hand) + c.hand = nil + } +} + // readFromUntil reads from r into c.rawInput until c.rawInput contains // at least n bytes or else returns an error. func (c *Conn) readFromUntil(r io.Reader, n int) error { + if c.rawInput == nil { + // The record buffer was released while waiting for a new + // record. Block for the header using the connection's small + // buffer; the switch to a pooled record-sized buffer below + // happens only once the payload length is known and data is + // flowing. + if c.smallInput == nil { + c.smallInput = new(bytes.Buffer) + } + c.rawInput = c.smallInput + } if c.rawInput.Len() >= n { return nil } needs := n - c.rawInput.Len() + if want := c.rawInput.Len() + needs + bytes.MinRead; want > maxIdleInputCap && want > c.rawInput.Cap() { + // Growing past maxIdleInputCap: switch to a pooled buffer so + // that record-sized buffers are recycled across connections + // rather than allocated for every record. + b := rawInputPool.Get().(*bytes.Buffer) + b.Write(c.rawInput.Bytes()) + if c.rawInput == c.smallInput { + c.smallInput.Reset() + } else if c.rawInput.Cap() > maxIdleInputCap { + c.rawInput.Reset() + rawInputPool.Put(c.rawInput) + } + c.rawInput = b + } // There might be extra input waiting on the wire. Make a best effort // attempt to fetch it so that it can be used in (*Conn).Read to // "predict" closeNotify alerts. @@ -1079,7 +1181,7 @@ func (c *Conn) readHandshakeBytes(n int) error { if c.quic != nil { return c.quicReadHandshakeBytes(n) } - for c.hand.Len() < n { + for c.handLen() < n { if err := c.readRecord(); err != nil { return err } @@ -1392,11 +1494,12 @@ func (c *Conn) Read(b []byte) (int, error) { if err := c.readRecord(); err != nil { return 0, err } - for c.hand.Len() > 0 { + for c.handLen() > 0 { if err := c.handlePostHandshakeMessage(); err != nil { return 0, err } } + c.releaseHand() } n, _ := c.input.Read(b) @@ -1574,6 +1677,14 @@ func (c *Conn) handshakeContext(ctx context.Context) (ret error) { panic("tls: internal error: handshake returned an error but is marked successful") } + // The handshake buffer typically grew to hold the peer's largest + // flight of handshake messages and is now empty. Post-handshake + // messages are rare and small, so release the buffer rather than + // pinning it for the life of the connection. + if c.handshakeErr == nil { + c.releaseHand() + } + if c.quic != nil { if c.handshakeErr == nil { c.quicHandshakeComplete() @@ -1685,7 +1796,7 @@ func (c *Conn) setReadTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptio // Ensure that there are no buffered handshake messages before changing the // read keys, since that can cause messages to be parsed that were encrypted // using old keys which are no longer appropriate. - if c.hand.Len() != 0 { + if c.handLen() != 0 { if locked { c.sendAlertLocked(alertUnexpectedMessage) } else { diff --git a/src/crypto/tls/handshake_server_tls13.go b/src/crypto/tls/handshake_server_tls13.go index 2c96e2435ae156..b5a7ea6ae4f1e5 100644 --- a/src/crypto/tls/handshake_server_tls13.go +++ b/src/crypto/tls/handshake_server_tls13.go @@ -528,7 +528,7 @@ func (hs *serverHandshakeStateTLS13) doHelloRetryRequest(selectedGroup CurveID) // Make sure the client didn't send extra handshake messages alongside // their initial client_hello. If they sent two client_hello messages, // we will consume the second before they respond to the server_hello. - if c.hand.Len() != 0 { + if c.handLen() != 0 { c.sendAlert(alertUnexpectedMessage) return nil, errors.New("tls: handshake buffer not empty before HelloRetryRequest") } diff --git a/src/crypto/tls/quic.go b/src/crypto/tls/quic.go index b872d4fe0174a0..6755ebf58dedd3 100644 --- a/src/crypto/tls/quic.go +++ b/src/crypto/tls/quic.go @@ -288,9 +288,9 @@ func (q *QUICConn) HandleData(level QUICEncryptionLevel, data []byte) error { // The handshake goroutine has exited. c.handshakeMutex.Lock() defer c.handshakeMutex.Unlock() - c.hand.Write(c.quic.readbuf) + c.handBuf().Write(c.quic.readbuf) c.quic.readbuf = nil - for q.conn.hand.Len() >= 4 && q.conn.handshakeErr == nil { + for q.conn.handLen() >= 4 && q.conn.handshakeErr == nil { b := q.conn.hand.Bytes() n := int(b[1])<<16 | int(b[2])<<8 | int(b[3]) if n > maxHandshake { @@ -304,6 +304,7 @@ func (q *QUICConn) HandleData(level QUICEncryptionLevel, data []byte) error { q.conn.handshakeErr = err } } + q.conn.releaseHand() if q.conn.handshakeErr != nil { return quicError(q.conn.handshakeErr) } @@ -394,7 +395,7 @@ func quicError(err error) error { } func (c *Conn) quicReadHandshakeBytes(n int) error { - for c.hand.Len() < n { + for c.handLen() < n { if err := c.quicWaitForSignal(); err != nil { return err } @@ -407,7 +408,7 @@ func (c *Conn) quicSetReadSecret(level QUICEncryptionLevel, suite uint16, secret // read keys, since that can cause messages to be parsed that were encrypted // using old keys which are no longer appropriate. // TODO(roland): we should merge this check with the similar one in setReadTrafficSecret. - if c.hand.Len() != 0 { + if c.handLen() != 0 { c.sendAlert(alertUnexpectedMessage) return errors.New("tls: handshake buffer not empty before setting read traffic secret") } @@ -520,7 +521,7 @@ func (c *Conn) quicWaitForSignal() error { // The connection has been canceled. return c.sendAlertLocked(alertCloseNotify) } - c.hand.Write(c.quic.readbuf) + c.handBuf().Write(c.quic.readbuf) c.quic.readbuf = nil return nil } From 493df8035b9cca81ae3d7cbffda1025ed1e3587e Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sat, 5 Sep 2026 21:27:36 +0000 Subject: [PATCH 07/10] [tailscale-cherrypick] net/http/internal/http2: turn Framer.getReadBuf func field into a method The field was never configurable: it was unexported and only ever set to its one default implementation by NewFramer, even in its previous life in x/net/http2. Make it a plain method, removing the per-Framer closure allocation and the indirection, and drop the stale TODO about making it configurable. No behavior change. Updates #80735 Updates tailscale/go#174 Change-Id: I694312b683a40d441e02faf089c08922f30bf946 Reviewed-on: https://go-review.googlesource.com/c/go/+/828204 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Nicholas Husin Reviewed-by: Nicholas Husin Auto-Submit: Damien Neil Reviewed-by: Damien Neil --- src/net/http/internal/http2/frame.go | 30 +++++++++++++++++----------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/net/http/internal/http2/frame.go b/src/net/http/internal/http2/frame.go index e3b371acec42a3..03606cfd8df9c9 100644 --- a/src/net/http/internal/http2/frame.go +++ b/src/net/http/internal/http2/frame.go @@ -296,11 +296,7 @@ type Framer struct { maxReadSize uint32 headerBuf [frameHeaderLen]byte - // TODO: let getReadBuf be configurable, and use a less memory-pinning - // allocator in server.go to minimize memory pinned for many idle conns. - // Will probably also need to make frame invalidation have a hook too. - getReadBuf func(size uint32) []byte - readBuf []byte // cache for default getReadBuf + readBuf []byte // cache for getReadBuf maxWriteSize uint32 // zero means unlimited; TODO: implement @@ -464,17 +460,27 @@ func NewFramer(w io.Writer, r io.Reader) *Framer { debugReadLoggerf: log.Printf, debugWriteLoggerf: log.Printf, } - fr.getReadBuf = func(size uint32) []byte { - if cap(fr.readBuf) >= int(size) { - return fr.readBuf[:size] - } - fr.readBuf = make([]byte, size) - return fr.readBuf - } fr.SetMaxReadFrameSize(maxFrameSize) return fr } +// getReadBuf returns a buffer of the given size for ReadFrameForHeader +// to read a frame payload into. The returned buffer is only used until +// the next getReadBuf call, but the frame returned to the caller +// aliases it, so the buffer must not be reused until the frame is +// invalidated. +// +// TODO: use a less memory-pinning allocation strategy here to minimize +// memory pinned for many idle conns. Will probably also need to make +// frame invalidation have a hook too. +func (fr *Framer) getReadBuf(size uint32) []byte { + if cap(fr.readBuf) >= int(size) { + return fr.readBuf[:size] + } + fr.readBuf = make([]byte, size) + return fr.readBuf +} + // SetMaxReadFrameSize sets the maximum size of a frame // that will be read by a subsequent call to ReadFrame. // It is the caller's responsibility to advertise this From 01416a5a36d7825bca410cc7c2702273bb7d4895 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sun, 6 Sep 2026 17:32:20 +0000 Subject: [PATCH 08/10] [tailscale-cherrypick] net/http: don't pin bufio buffers on idle HTTP/1 keep-alive connections As part of my recent mission to reduce pinned memory during idle blocked reads, this addresses the bulk of HTTP/1.1 memory wastage. We already had bufio memory pools in this package, but they weren't being used in this case. After we do the first HTTP/1 request on a connection and are about to read the second, we already had a read timeout on the connection (the idle timeout). That next request might be ready ~right away, or it might time out. This splits up that idle timeout into two phases: a quick little timeout to see if the server's hot enough to have a new request ready, and the the remainder of the time we would've waited anyway. If we don't get a new HTTP request ~right away (50 ms, currently), then we release our bufios (read+write) to the existing pools and enter our happy memory-released long idle. But on hot servers, no new pool machinery is involved and the same bufios remain. Measured with 1000 idle HTTP/1.1 keep-alive TLS connections after POST exchanges with 16 kB uploads and 4 kB responses (heap bytes per connection, runtime.MemProfileRate=1): HeapAlloc/conn: 13852 B -> 5335 B Updates #80735 Updates tailscale/go#174 Change-Id: I64b27bb7f1a65d9cb65cc0c38ebc1faa6c5a7682 Reviewed-on: https://go-review.googlesource.com/c/go/+/828504 Reviewed-by: Nicholas Husin LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Auto-Submit: Damien Neil Reviewed-by: Nicholas Husin Reviewed-by: Damien Neil --- src/net/http/server.go | 81 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 77 insertions(+), 4 deletions(-) diff --git a/src/net/http/server.go b/src/net/http/server.go index aafd15401686bd..f2116101af70c2 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -23,6 +23,7 @@ import ( "net/textproto" "net/url" urlpkg "net/url" + "os" "path" "runtime" "slices" @@ -782,6 +783,46 @@ func (cr *connReader) backgroundRead() { cr.cond.Broadcast() } +// idleBufsReleaseDelay is how long a keep-alive connection waits for +// its next request before it is considered idle and its bufio buffers +// are released to their pools. It trades a little extra work on +// connections that idle past it against pinning ~8 kB of buffers on +// every waiting connection. +const idleBufsReleaseDelay = 50 * time.Millisecond + +// waitReadable blocks until data arrives on the connection, stashing +// the byte it reads for the next connReader.Read, and reports whether +// data arrived. It is called between requests, after the connection's +// bufio buffers have been released to their pools, so that an idle +// connection pins no buffer memory while it waits, possibly for a long +// time, for the next request. A false return means the read failed +// (EOF, a timeout, or another error) and the error has been handled by +// handleReadErrorLocked. +func (cr *connReader) waitReadable() (readable bool) { + cr.lock() + if cr.inRead { + panic("invalid concurrent connReader.waitReadable call") + } + if cr.hasByte { + cr.unlock() + return true + } + cr.inRead = true + cr.unlock() + n, err := cr.rwc.Read(cr.byteBuf[:]) + cr.lock() + cr.inRead = false + if n == 1 { + cr.hasByte = true + } + if err != nil { + cr.handleReadErrorLocked(err) + } + cr.unlock() + cr.cond.Broadcast() + return n == 1 && err == nil +} + func (cr *connReader) abortPendingRead() { cr.lock() defer cr.unlock() @@ -2163,17 +2204,49 @@ func (c *conn) serve(ctx context.Context) { return } + var idleDeadline time.Time if d := c.server.idleTimeout(); d > 0 { - c.rwc.SetReadDeadline(time.Now().Add(d)) - } else { - c.rwc.SetReadDeadline(time.Time{}) + idleDeadline = time.Now().Add(d) } // Wait for the connection to become readable again before trying to // read the next request. This prevents a ReadHeaderTimeout or // ReadTimeout from starting until the first bytes of the next request // have been received. - if _, err := c.bufr.Peek(4); err != nil { + // + // The wait runs in two phases. First wait briefly with the + // connection's bufio buffers still attached: on a busy + // connection the next request typically arrives almost + // immediately, and this keeps the buffer release below off the + // hot path. If the connection then still has nothing buffered, + // it has gone idle, possibly for a long time, so release its + // bufio.Reader and Writer (~8 kB of per-connection memory + // holding no data) to their pools for the rest of the wait. + // The byte read by waitReadable is stashed in the connReader + // and yielded by its next Read after fresh buffers are + // acquired. + shortDeadline := time.Now().Add(idleBufsReleaseDelay) + if !idleDeadline.IsZero() && idleDeadline.Before(shortDeadline) { + shortDeadline = idleDeadline + } + c.rwc.SetReadDeadline(shortDeadline) + _, peekErr := c.bufr.Peek(4) + if errors.Is(peekErr, os.ErrDeadlineExceeded) && (idleDeadline.IsZero() || time.Now().Before(idleDeadline)) { + c.rwc.SetReadDeadline(idleDeadline) + if c.bufr.Buffered() == 0 && c.bufw.Buffered() == 0 { + putBufioReader(c.bufr) + c.bufr = nil + putBufioWriter(c.bufw) + c.bufw = nil + if !c.r.waitReadable() { + return + } + c.bufr = newBufioReader(c.r) + c.bufw = newBufioWriterSize(checkConnErrorWriter{c}, 4<<10) + } + _, peekErr = c.bufr.Peek(4) + } + if peekErr != nil { return } From d35527162c4bc3eeeab81bcb087d771e80266ddd Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sat, 5 Sep 2026 01:20:29 +0000 Subject: [PATCH 09/10] [tailscale-cherrypick] net/http/internal/http2: don't pin frame-sized read buffers on idle conns A Framer's readBuf grows to the size of the largest frame received (16 kB with the default max frame size) and was retained for the life of the connection, including while blocked in ReadFrame waiting, often for minutes or hours on long-polling or mostly idle connections, for the next 9-byte frame header. Servers with many thousands of open HTTP/2 connections strand most of their per-connection memory in these buffers holding no data. The previous frame is already documented as invalid as of the next ReadFrame call, so release the read buffer to a sync.Pool at the top of ReadFrame, before blocking in ReadFrameHeader, and get one back only once a frame header with a large payload length arrives. Buffers up to 1 kB stay attached to their Framer, so connections receiving only small control frames don't pay for the pooling, and if the Framer's reader is a bufio.Reader with data already buffered (the Transport's case, mid-stream), the buffer is kept since no blocking wait is about to happen. Measured with 1000 idle HTTP/2 connections on separate TCP flows, each blocked reading a frame header after POST exchanges with 16 kB bodies (heap bytes per connection, runtime.MemProfileRate=1): HeapAlloc/conn: 32715 B -> 16363 B HeapInuse/conn: 40779 B -> 22216 B The largest remaining consumers are the Framer's write buffer (~4.8 kB), the serverConn struct (~1.8 kB), TLS cipher state (~1.8 kB), and the HPACK tables (~1 kB). Updates #80735 Updates tailscale/go#174 Change-Id: I3d3fd1a237b90e716b708789dea40e3eeb0dbd25 Reviewed-on: https://go-review.googlesource.com/c/go/+/828205 Reviewed-by: Damien Neil Reviewed-by: Nicholas Husin Reviewed-by: Nicholas Husin LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com --- src/net/http/internal/http2/frame.go | 85 ++++++++++++++-- src/net/http/internal/http2/frame_test.go | 118 ++++++++++++++++++++++ 2 files changed, 194 insertions(+), 9 deletions(-) diff --git a/src/net/http/internal/http2/frame.go b/src/net/http/internal/http2/frame.go index 03606cfd8df9c9..4632259f3d404e 100644 --- a/src/net/http/internal/http2/frame.go +++ b/src/net/http/internal/http2/frame.go @@ -5,6 +5,7 @@ package http2 import ( + "bufio" "bytes" "encoding/binary" "errors" @@ -296,7 +297,16 @@ type Framer struct { maxReadSize uint32 headerBuf [frameHeaderLen]byte - readBuf []byte // cache for getReadBuf + // readBuf is the buffer most recently returned by getReadBuf, + // reused for subsequent frames that fit in it. releaseReadBuf + // drops it before blocking to wait for a new frame header, so + // idle connections don't pin a frame-sized buffer. + readBuf []byte + // readBufP, if non-nil, is the pooled container holding readBuf's + // array, to be returned to readBufPool by releaseReadBuf. It is + // nil when readBuf is small (under maxIdleReadBufCap) and not + // worth pooling. + readBufP *[]byte maxWriteSize uint32 // zero means unlimited; TODO: implement @@ -468,19 +478,74 @@ func NewFramer(w io.Writer, r io.Reader) *Framer { // to read a frame payload into. The returned buffer is only used until // the next getReadBuf call, but the frame returned to the caller // aliases it, so the buffer must not be reused until the frame is -// invalidated. -// -// TODO: use a less memory-pinning allocation strategy here to minimize -// memory pinned for many idle conns. Will probably also need to make -// frame invalidation have a hook too. +// invalidated. The buffer is cached in readBuf, and large buffers are +// backed by readBufPool via readBufP. func (fr *Framer) getReadBuf(size uint32) []byte { if cap(fr.readBuf) >= int(size) { return fr.readBuf[:size] } + if int(size) > maxIdleReadBufCap { + // Reuse the pooled container we already hold, if any; + // otherwise get one from the pool. + bp := fr.readBufP + if bp == nil { + bp, _ = readBufPool.Get().(*[]byte) + if bp == nil { + bp = new([]byte) + } + } + if cap(*bp) < int(size) { + *bp = make([]byte, size) // drop the old array (don't Put it back), so the pool converges toward large buffers + } + fr.readBufP = bp + fr.readBuf = (*bp)[:size] + return fr.readBuf + } + fr.readBufP = nil fr.readBuf = make([]byte, size) return fr.readBuf } +// readBufPool pools the frame-sized buffers that back Framer.readBuf. +// A Framer returns its buffer to the pool before blocking to wait for +// a new frame header, often for minutes or hours on idle connections, +// so that each idle connection does not pin a frame-sized buffer. +var readBufPool sync.Pool + +// maxIdleReadBufCap is the largest readBuf capacity that a Framer keeps +// while blocked waiting for a frame header to arrive. It is large +// enough for control frames and small HEADERS frames, so that only +// connections receiving larger frames pay for the buffer pooling. +const maxIdleReadBufCap = 1024 + +// releaseReadBuf invalidates the frame returned by the previous read +// and, if the read buffer came from readBufPool, returns it to the +// pool. It is called before blocking to wait for a new frame header. +func (fr *Framer) releaseReadBuf() { + if fr.readBufP == nil { + return + } + if br, ok := fr.r.(*bufio.Reader); ok && br.Buffered() > 0 { + // More frame data is already buffered: we are not about to + // block, and the buffer is about to be needed again. + return + } + fr.invalidateLastFrame() + *fr.readBufP = fr.readBuf[:0] + readBufPool.Put(fr.readBufP) + fr.readBufP = nil + fr.readBuf = nil +} + +// invalidateLastFrame invalidates the frame returned by the previous +// ReadFrameForHeader call, if any. +func (fr *Framer) invalidateLastFrame() { + if fr.lastFrame != nil { + fr.lastFrame.invalidate() + fr.lastFrame = nil + } +} + // SetMaxReadFrameSize sets the maximum size of a frame // that will be read by a subsequent call to ReadFrame. // It is the caller's responsibility to advertise this @@ -549,9 +614,7 @@ func (fr *Framer) ReadFrameHeader() (FrameHeader, error) { // It behaves identically to ReadFrame, other than not checking the maximum // frame size. func (fr *Framer) ReadFrameForHeader(fh FrameHeader) (Frame, error) { - if fr.lastFrame != nil { - fr.lastFrame.invalidate() - } + fr.invalidateLastFrame() payload := fr.getReadBuf(fh.Length) if _, err := io.ReadFull(fr.r, payload); err != nil { if fh == invalidHTTP1LookingFrameHeader() { @@ -587,6 +650,10 @@ func (fr *Framer) ReadFrameForHeader(fh FrameHeader) (Frame, error) { // If ReadFrame returns an error and a non-nil Frame, the Frame's StreamID // indicates the stream responsible for the error. func (fr *Framer) ReadFrame() (Frame, error) { + // The previous frame is invalid as of this call, so don't pin a + // frame-sized read buffer while waiting, possibly for a long time, + // for the next frame to arrive. + fr.releaseReadBuf() fh, err := fr.ReadFrameHeader() if err != nil { return nil, err diff --git a/src/net/http/internal/http2/frame_test.go b/src/net/http/internal/http2/frame_test.go index 43cd66bfc3bdc0..009e0ad054f3b8 100644 --- a/src/net/http/internal/http2/frame_test.go +++ b/src/net/http/internal/http2/frame_test.go @@ -5,6 +5,7 @@ package http2 import ( + "bufio" "bytes" "fmt" "io" @@ -1605,3 +1606,120 @@ func TestTypeFrameParserHolePanic(t *testing.T) { t.Errorf("got %T; want *UnknownFrame", f) } } + +// frameAccessPanics reports whether accessing the given frame's payload +// panics, as it must after the frame has been invalidated by a +// subsequent ReadFrame call. +func frameAccessPanics(f func()) (panicked bool) { + defer func() { panicked = recover() != nil }() + f() + return +} + +func TestFramerReadBufPooling(t *testing.T) { + fr, _ := testFramer() + + // Reading a frame larger than maxIdleReadBufCap acquires a pooled buffer. + fr.WriteData(1, false, make([]byte, maxIdleReadBufCap+1)) + f1, err := fr.ReadFrame() + if err != nil { + t.Fatal(err) + } + if fr.readBufP == nil { + t.Error("after large frame: readBufP = nil, want pooled buffer") + } + if cap(fr.readBuf) <= maxIdleReadBufCap { + t.Errorf("after large frame: cap(readBuf) = %d, want > %d", cap(fr.readBuf), maxIdleReadBufCap) + } + + // The next ReadFrame releases the pooled buffer before blocking + // (bytes.Buffer is not a bufio.Reader, so there is no buffered + // look-ahead), invalidates the previous frame, and reads a payload + // of maxIdleReadBufCap or less into a small non-pooled buffer. + fr.WriteData(1, false, make([]byte, maxIdleReadBufCap)) + if _, err := fr.ReadFrame(); err != nil { + t.Fatal(err) + } + if fr.readBufP != nil { + t.Error("after small frame: readBufP != nil, want small non-pooled buffer") + } + if cap(fr.readBuf) > maxIdleReadBufCap { + t.Errorf("after small frame: cap(readBuf) = %d, want <= %d", cap(fr.readBuf), maxIdleReadBufCap) + } + if !frameAccessPanics(func() { f1.(*DataFrame).Data() }) { + t.Error("accessing previous frame's Data after ReadFrame did not panic") + } +} + +func TestFramerReadBufKeptWhileBuffered(t *testing.T) { + var wire bytes.Buffer + fw := NewFramer(&wire, nil) + fw.WriteData(1, false, make([]byte, maxIdleReadBufCap+1)) + fw.WriteData(1, false, make([]byte, maxIdleReadBufCap+1)) + + // With a bufio.Reader that already has the next frame buffered, + // ReadFrame is not about to block and keeps the pooled buffer. + fr := NewFramer(nil, bufio.NewReader(bytes.NewReader(wire.Bytes()))) + if _, err := fr.ReadFrame(); err != nil { + t.Fatal(err) + } + p1 := fr.readBufP + if p1 == nil { + t.Fatal("after large frame: readBufP = nil, want pooled buffer") + } + if _, err := fr.ReadFrame(); err != nil { + t.Fatal(err) + } + if fr.readBufP != p1 { + t.Error("pooled buffer was released despite buffered look-ahead data") + } +} + +func TestFramerReadBufGrowsInPlace(t *testing.T) { + var wire bytes.Buffer + fw := NewFramer(&wire, nil) + fw.WriteData(1, false, make([]byte, 2*maxIdleReadBufCap)) + fw.WriteData(1, false, make([]byte, 4*maxIdleReadBufCap)) + + // A larger frame arriving while the pooled buffer is retained + // (buffered look-ahead, so no release) grows the buffer within the + // same pooled container rather than acquiring a second one. + fr := NewFramer(nil, bufio.NewReader(bytes.NewReader(wire.Bytes()))) + if _, err := fr.ReadFrame(); err != nil { + t.Fatal(err) + } + p1 := fr.readBufP + if p1 == nil { + t.Fatal("after first frame: readBufP = nil, want pooled buffer") + } + if _, err := fr.ReadFrame(); err != nil { + t.Fatal(err) + } + if fr.readBufP != p1 { + t.Error("readBufP container not reused for larger frame") + } + if cap(fr.readBuf) < 4*maxIdleReadBufCap { + t.Errorf("cap(readBuf) = %d, want >= %d", cap(fr.readBuf), 4*maxIdleReadBufCap) + } +} + +func TestFramerReadBufReleasedOnReadError(t *testing.T) { + fr, _ := testFramer() + fr.WriteData(1, false, make([]byte, maxIdleReadBufCap+1)) + f1, err := fr.ReadFrame() + if err != nil { + t.Fatal(err) + } + + // A ReadFrame call that fails reading the next header still + // releases the buffer and invalidates the previous frame first. + if _, err := fr.ReadFrame(); err != io.EOF { + t.Fatalf("ReadFrame on empty input = %v, want io.EOF", err) + } + if fr.readBufP != nil { + t.Error("readBufP != nil after ReadFrame error, want released") + } + if !frameAccessPanics(func() { f1.(*DataFrame).Data() }) { + t.Error("accessing previous frame's Data after failed ReadFrame did not panic") + } +} From f147da0b5d9227fe716195da0ba93721457e5bd8 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sat, 5 Sep 2026 15:33:22 +0000 Subject: [PATCH 10/10] [tailscale-cherrypick] net/http/internal/http2: don't pin frame-sized write buffers on idle conns A Framer's wbuf grows to the size of the largest frame written (typically a full DATA frame) and was retained for the life of the connection, pinning several kB per idle connection. The frame is fully written out by the end of endWrite, so release large write buffers to a sync.Pool there and have startWrite adopt a pooled buffer for the next frame. Buffers up to 1 kB stay attached to their Framer, so connections writing only small control frames don't pay for the pooling. Measured with 1000 idle HTTP/2 connections on separate TCP flows after POST exchanges with 4 kB responses (heap bytes per connection, runtime.MemProfileRate=1, HPACK dynamic tables disabled, and with the corresponding read buffer fix already applied): HeapAlloc/conn: 15265 B -> 10377 B HeapInuse/conn: 20357 B -> 15114 B http2 benchmarks are unchanged within noise. The largest remaining consumers are the serverConn struct (~1.8 kB), TLS cipher state (~1.8 kB), and the tls.Conn struct (~0.9 kB). Updates #80735 Updates tailscale/go#174 Change-Id: I7c1e08e2f6df944d34a2ef7a5f5091b1d3648b95 Reviewed-on: https://go-review.googlesource.com/c/go/+/828206 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Damien Neil Reviewed-by: Nicholas Husin Reviewed-by: Nicholas Husin --- src/net/http/internal/http2/frame.go | 52 ++++++++++++++++++++++- src/net/http/internal/http2/frame_test.go | 42 ++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/src/net/http/internal/http2/frame.go b/src/net/http/internal/http2/frame.go index 4632259f3d404e..5567c2ae97a164 100644 --- a/src/net/http/internal/http2/frame.go +++ b/src/net/http/internal/http2/frame.go @@ -310,8 +310,16 @@ type Framer struct { maxWriteSize uint32 // zero means unlimited; TODO: implement - w io.Writer + w io.Writer + // wbuf is the frame currently being written, from startWrite's + // header through endWrite backfilling the length and writing it + // out. endWrite releases large buffers to writeBufPool so that + // idle connections don't pin a frame-sized buffer. wbuf []byte + // wbufP, if non-nil, is the pooled container holding wbuf's array, + // to be returned to writeBufPool by endWrite. It is nil when wbuf + // is small (under maxIdleWriteBufCap) and not worth pooling. + wbufP *[]byte // AllowIllegalWrites permits the Framer's Write methods to // write frames that do not conform to the HTTP/2 spec. This @@ -372,7 +380,27 @@ func (fr *Framer) maxHeaderValueCount() int { return fr.MaxHeaderValueCount } +// writeBufPool pools the frame-sized buffers that back Framer.wbuf. +// endWrite returns the buffer to the pool once the frame has been +// written out, so that idle connections don't each pin a buffer sized +// by the last frame they wrote. +var writeBufPool sync.Pool + +// maxIdleWriteBufCap is the largest wbuf capacity that a Framer keeps +// between frame writes. It is large enough for control frames and +// small HEADERS frames, so that only connections writing larger frames +// pay for the buffer pooling. +const maxIdleWriteBufCap = 1024 + func (f *Framer) startWrite(ftype FrameType, flags Flags, streamID uint32) { + if f.wbuf == nil { + // endWrite released the previous buffer; start with a pooled + // one if available. Appends below grow it as needed. + if bp, _ := writeBufPool.Get().(*[]byte); bp != nil { + f.wbufP = bp + f.wbuf = (*bp)[:0] + } + } // Write the FrameHeader. f.wbuf = append(f.wbuf[:0], 0, // 3 bytes of length, filled in endWrite @@ -405,9 +433,31 @@ func (f *Framer) endWrite() error { if err == nil && n != len(f.wbuf) { err = io.ErrShortWrite } + f.releaseWriteBuf() return err } +// releaseWriteBuf returns a large wbuf, whose frame has been written +// out, to writeBufPool, so that a connection idle between writes does +// not pin a frame-sized buffer. Small buffers stay attached to the +// Framer. +func (f *Framer) releaseWriteBuf() { + if cap(f.wbuf) <= maxIdleWriteBufCap { + return + } + bp := f.wbufP + if bp == nil { + bp = new([]byte) + } + // If appends outgrew a pooled array, the container is recycled + // with the new larger array and the old one is dropped, so the + // pool converges toward large buffers. + *bp = f.wbuf[:0] + writeBufPool.Put(bp) + f.wbufP = nil + f.wbuf = nil +} + func (f *Framer) logWrite() { if f.debugFramer == nil { f.debugFramerBuf = new(bytes.Buffer) diff --git a/src/net/http/internal/http2/frame_test.go b/src/net/http/internal/http2/frame_test.go index 009e0ad054f3b8..2d3318be9695c4 100644 --- a/src/net/http/internal/http2/frame_test.go +++ b/src/net/http/internal/http2/frame_test.go @@ -1723,3 +1723,45 @@ func TestFramerReadBufReleasedOnReadError(t *testing.T) { t.Error("accessing previous frame's Data after failed ReadFrame did not panic") } } + +func TestFramerWriteBufReleased(t *testing.T) { + fr, buf := testFramer() + if err := fr.WriteData(1, false, make([]byte, 2*maxIdleWriteBufCap)); err != nil { + t.Fatal(err) + } + if fr.wbuf != nil || fr.wbufP != nil { + t.Errorf("after large write: wbuf = %v (cap %d), wbufP = %v; want both released", + fr.wbuf != nil, cap(fr.wbuf), fr.wbufP != nil) + } + + // Subsequent writes still produce well-formed frames. + if err := fr.WritePing(false, [8]byte{1, 2, 3, 4, 5, 6, 7, 8}); err != nil { + t.Fatal(err) + } + if _, err := fr.ReadFrame(); err != nil { // the DATA frame + t.Fatal(err) + } + f, err := fr.ReadFrame() + if err != nil { + t.Fatal(err) + } + if _, ok := f.(*PingFrame); !ok { + t.Errorf("second frame = %T, want *PingFrame", f) + } + _ = buf +} + +func TestFramerWriteBufSmallKept(t *testing.T) { + fr, _ := testFramer() + fr.wbuf = make([]byte, 0, 64) // small buffer stays attached across writes + if err := fr.WritePing(false, [8]byte{}); err != nil { + t.Fatal(err) + } + if fr.wbuf == nil || cap(fr.wbuf) != 64 { + t.Errorf("after small write: cap(wbuf) = %d (nil=%v), want the seeded 64-byte buffer kept", + cap(fr.wbuf), fr.wbuf == nil) + } + if fr.wbufP != nil { + t.Error("after small write: wbufP != nil, want nil") + } +}