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) + } + } + }) +}