From c2f0718d6e9c3cb9a0a943cb60085d1c669f1ef3 Mon Sep 17 00:00:00 2001 From: Burrup Lambert Date: Fri, 31 Jul 2026 16:53:14 +0100 Subject: [PATCH] fix: do not mutate the caller's exclude map in writeSubset, drop global mutex writeSubset wrote the Header-Order:/PHeader-Order: magic keys into the caller's exclude map. Callers such as Response.Write and httputil dump pass shared package-level maps, so that write both raced with concurrent header writes and permanently leaked the exclusions into every later write using the same map - an orderless write's output could depend on whether an ordered write had happened earlier in the process. The package-global RWMutex (added in 7abeb12 and 107c8e4) exists only to guard that mutation, at the cost of two global lock operations per header key on every sort, shared across all connections. Instead, add the magic keys to a small copy of the exclude map in the one path that needs them and never write to the caller's map. The mutex then guards nothing and is removed along with the per-key RLock/RUnlock in SortedKeyValues and SortedKeyValuesBy. Wire output is unchanged for every fresh-process input; the only behavioral change is that orderless writes no longer depend on the process's write history, which was unintended state leakage. Adds a test pinning that WriteSubset does not mutate the caller's map, a shared-map concurrency test for the race detector, and a parallel WriteSubset benchmark (the mutex cost only shows under concurrency: about -28% ns/op on 2 cores with it removed). --- header.go | 25 +++++++-------- header_test.go | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 14 deletions(-) diff --git a/header.go b/header.go index b6481131..17265fe1 100644 --- a/header.go +++ b/header.go @@ -6,6 +6,7 @@ package http import ( "io" + "maps" "net/textproto" "sort" "strings" @@ -204,8 +205,6 @@ var headerSorterPool = sync.Pool{ New: func() interface{} { return new(headerSorter) }, } -var mutex = &sync.RWMutex{} - // SortedKeyValues returns h's keys sorted in the returned kvs // slice. The headerSorter used to sort is also returned, for possible // return to headerSorterCache. @@ -216,11 +215,9 @@ func (h Header) SortedKeyValues(exclude map[string]bool) (kvs []HeaderKeyValues, } kvs = hs.kvs[:0] for k, vv := range h { - mutex.RLock() if !exclude[k] { kvs = append(kvs, HeaderKeyValues{k, vv}) } - mutex.RUnlock() } hs.kvs = kvs sort.Sort(hs) @@ -234,11 +231,9 @@ func (h Header) SortedKeyValuesBy(order map[string]int, exclude map[string]bool) } kvs = hs.kvs[:0] for k, vv := range h { - mutex.RLock() if !exclude[k] { kvs = append(kvs, HeaderKeyValues{k, vv}) } - mutex.RUnlock() } hs.kvs = kvs hs.order = order @@ -269,14 +264,16 @@ func (h Header) writeSubset(w io.Writer, exclude map[string]bool, trace *httptra for i, v := range headerOrder { order[v] = i } - if exclude == nil { - exclude = make(map[string]bool) - } - mutex.Lock() - exclude[HeaderOrderKey] = true - exclude[PHeaderOrderKey] = true - mutex.Unlock() - kvs, sorter = h.SortedKeyValuesBy(order, exclude) + // Add the magic keys to a copy of exclude instead of mutating the + // caller's map: callers pass shared package-level maps (e.g. + // respExcludeHeader), so writing to exclude both raced with other + // writers and readers and leaked the exclusions into every later + // write that used the same map. + excl := make(map[string]bool, len(exclude)+2) + maps.Copy(excl, exclude) + excl[HeaderOrderKey] = true + excl[PHeaderOrderKey] = true + kvs, sorter = h.SortedKeyValuesBy(order, excl) } else { kvs, sorter = h.SortedKeyValues(exclude) } diff --git a/header_test.go b/header_test.go index 3d9361bd..bb9848d7 100644 --- a/header_test.go +++ b/header_test.go @@ -6,8 +6,10 @@ package http import ( "bytes" + "io" "reflect" "runtime" + "sync" "testing" "time" @@ -359,3 +361,85 @@ func TestHTTP1HeaderOrder(t *testing.T) { t.Fatalf("got:\n%swant:\n%s", buf.String(), expected) } } + +func TestWriteSubsetDoesNotMutateExclude(t *testing.T) { + h := Header{ + "Keep-One": {"1"}, + "Drop-Me": {"nope"}, + "Keep-Two": {"2"}, + HeaderOrderKey: {"keep-two", "keep-one"}, + PHeaderOrderKey: {":method"}, + } + exclude := map[string]bool{"Drop-Me": true} + + var buf bytes.Buffer + if err := h.WriteSubset(&buf, exclude); err != nil { + t.Fatal(err) + } + + if want := map[string]bool{"Drop-Me": true}; !reflect.DeepEqual(exclude, want) { + t.Errorf("WriteSubset mutated the caller's exclude map: got %v, want %v", exclude, want) + } + got := buf.String() + if want := "Keep-Two: 2\r\nKeep-One: 1\r\n"; got != want { + t.Errorf("WriteSubset output = %q, want %q", got, want) + } +} + +func TestWriteSubsetSharedExcludeConcurrent(t *testing.T) { + // Callers such as Response.Write pass a shared package-level exclude + // map. Concurrent writes with and without a Header-Order: key must + // not race on it (this test is only meaningful under -race). + shared := map[string]bool{"Content-Length": true} + ordered := Header{ + "B-Second": {"2"}, + "A-First": {"1"}, + HeaderOrderKey: {"a-first", "b-second"}, + } + plain := Header{ + "Zulu": {"1"}, + "Alpha": {"2"}, + } + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + h := ordered + if i%2 == 1 { + h = plain + } + wg.Add(1) + go func() { + defer wg.Done() + for n := 0; n < 100; n++ { + if err := h.WriteSubset(io.Discard, shared); err != nil { + t.Error(err) + } + } + }() + } + wg.Wait() +} + +func BenchmarkHeaderWriteSubsetParallel(b *testing.B) { + b.ReportAllocs() + h := Header{ + "sec-ch-ua": {"\"Chromium\";v=\"124\""}, + "accept": {"*/*"}, + "user-agent": {"Mozilla/5.0"}, + "content-type": {"application/json"}, + "accept-language": {"en-US,en;q=0.9"}, + "accept-encoding": {"gzip, deflate, br"}, + "referer": {"https://example.org/x"}, + HeaderOrderKey: { + "sec-ch-ua", "accept", "user-agent", "content-type", + "referer", "accept-encoding", "accept-language", + }, + } + b.RunParallel(func(pb *testing.PB) { + for pb.Next() { + if err := h.WriteSubset(io.Discard, nil); err != nil { + b.Fatal(err) + } + } + }) +}