From f8411da775a685be247bbedcb3ed2c998f895cd2 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Thu, 5 Jan 2023 22:57:37 +0700 Subject: [PATCH 1/8] nettest: fix tests on dragonfly and js/wasm CL 458096 changes probeStack to use a better approach for checking network stack capability, by checking for routable ipv4/ipv6. However, the NewLocalListener needs check for listenable instead. This CL adds to probestack the listenable on loopback and use that condition instead. Fixes golang/go#57623 Change-Id: I8b5b7798ccf3826881e5ef9f7d2d998d8e52eba5 Reviewed-on: https://go-review.googlesource.com/c/net/+/460735 Run-TryBot: Cuong Manh Le TryBot-Result: Gopher Robot Auto-Submit: Cuong Manh Le Reviewed-by: Bryan Mills Reviewed-by: David Chase --- nettest/nettest.go | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/nettest/nettest.go b/nettest/nettest.go index 3d970bfc67..510555ac28 100644 --- a/nettest/nettest.go +++ b/nettest/nettest.go @@ -20,11 +20,13 @@ import ( ) var ( - stackOnce sync.Once - ipv4Enabled bool - ipv6Enabled bool - unStrmDgramEnabled bool - rawSocketSess bool + stackOnce sync.Once + ipv4Enabled bool + canListenTCP4OnLoopback bool + ipv6Enabled bool + canListenTCP6OnLoopback bool + unStrmDgramEnabled bool + rawSocketSess bool aLongTimeAgo = time.Unix(233431200, 0) neverTimeout = time.Time{} @@ -37,9 +39,17 @@ func probeStack() { if _, err := RoutedInterface("ip4", net.FlagUp); err == nil { ipv4Enabled = true } + if ln, err := net.Listen("tcp4", "127.0.0.1:0"); err == nil { + ln.Close() + canListenTCP4OnLoopback = true + } if _, err := RoutedInterface("ip6", net.FlagUp); err == nil { ipv6Enabled = true } + if ln, err := net.Listen("tcp6", "[::1]:0"); err == nil { + ln.Close() + canListenTCP6OnLoopback = true + } rawSocketSess = supportsRawSocket() switch runtime.GOOS { case "aix": @@ -152,22 +162,23 @@ func TestableAddress(network, address string) bool { // The provided network must be "tcp", "tcp4", "tcp6", "unix" or // "unixpacket". func NewLocalListener(network string) (net.Listener, error) { + stackOnce.Do(probeStack) switch network { case "tcp": - if SupportsIPv4() { + if canListenTCP4OnLoopback { if ln, err := net.Listen("tcp4", "127.0.0.1:0"); err == nil { return ln, nil } } - if SupportsIPv6() { + if canListenTCP6OnLoopback { return net.Listen("tcp6", "[::1]:0") } case "tcp4": - if SupportsIPv4() { + if canListenTCP4OnLoopback { return net.Listen("tcp4", "127.0.0.1:0") } case "tcp6": - if SupportsIPv6() { + if canListenTCP6OnLoopback { return net.Listen("tcp6", "[::1]:0") } case "unix", "unixpacket": @@ -185,22 +196,23 @@ func NewLocalListener(network string) (net.Listener, error) { // // The provided network must be "udp", "udp4", "udp6" or "unixgram". func NewLocalPacketListener(network string) (net.PacketConn, error) { + stackOnce.Do(probeStack) switch network { case "udp": - if SupportsIPv4() { + if canListenTCP4OnLoopback { if c, err := net.ListenPacket("udp4", "127.0.0.1:0"); err == nil { return c, nil } } - if SupportsIPv6() { + if canListenTCP6OnLoopback { return net.ListenPacket("udp6", "[::1]:0") } case "udp4": - if SupportsIPv4() { + if canListenTCP4OnLoopback { return net.ListenPacket("udp4", "127.0.0.1:0") } case "udp6": - if SupportsIPv6() { + if canListenTCP6OnLoopback { return net.ListenPacket("udp6", "[::1]:0") } case "unixgram": From 296f09aa3817abc1ddff7703799bf9babb7bbd16 Mon Sep 17 00:00:00 2001 From: Michael Fraenkel Date: Fri, 20 Jan 2023 15:28:00 -0700 Subject: [PATCH 2/8] http2: case insensitive handling for 100-continue rfc 9110, section 10.1.1 states that the Expect field value is case-insensitive. Fixes golang/go#57824 Change-Id: Ie0e2662c58a2933087e0d35935c04ec61026a41d Reviewed-on: https://go-review.googlesource.com/c/net/+/463096 Auto-Submit: Damien Neil Run-TryBot: Damien Neil Reviewed-by: Damien Neil Reviewed-by: Matthew Dempsky TryBot-Result: Gopher Robot --- http2/server.go | 2 +- http2/server_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/http2/server.go b/http2/server.go index b624dc0a70..9bd7035bfe 100644 --- a/http2/server.go +++ b/http2/server.go @@ -2192,7 +2192,7 @@ func (sc *serverConn) newWriterAndRequestNoBody(st *stream, rp requestParam) (*r tlsState = sc.tlsState } - needsContinue := rp.header.Get("Expect") == "100-continue" + needsContinue := httpguts.HeaderValuesContainsToken(rp.header["Expect"], "100-continue") if needsContinue { rp.header.Del("Expect") } diff --git a/http2/server_test.go b/http2/server_test.go index 178c28b058..fd62dcb931 100644 --- a/http2/server_test.go +++ b/http2/server_test.go @@ -2332,7 +2332,7 @@ func TestServer_Response_Automatic100Continue(t *testing.T) { }, func(st *serverTester) { st.writeHeaders(HeadersFrameParam{ StreamID: 1, // clients send odd numbers - BlockFragment: st.encodeHeader(":method", "POST", "expect", "100-continue"), + BlockFragment: st.encodeHeader(":method", "POST", "expect", "100-Continue"), EndStream: false, EndHeaders: true, }) From 7e3c19ca52e202ae203b1914fc00c8e47a4d72fa Mon Sep 17 00:00:00 2001 From: Oleksandr Redko Date: Mon, 6 Feb 2023 18:06:31 +0000 Subject: [PATCH 3/8] all: correct typos in comments Change-Id: Idc75240e5546be2f2b091878910339b4967c93c7 GitHub-Last-Rev: c78560c06fce951fefdb83c2550bc99ca15c48ef GitHub-Pull-Request: golang/net#166 Reviewed-on: https://go-review.googlesource.com/c/net/+/465715 Run-TryBot: Ian Lance Taylor Reviewed-by: David Chase Reviewed-by: Ian Lance Taylor Auto-Submit: Ian Lance Taylor TryBot-Result: Gopher Robot --- http2/server_test.go | 2 +- icmp/multipart.go | 2 +- internal/sockstest/server.go | 6 +++--- ipv4/multicastlistener_test.go | 2 +- ipv6/dgramopt.go | 2 +- ipv6/multicastlistener_test.go | 2 +- netutil/listen.go | 2 +- webdav/webdav.go | 2 +- websocket/hybi.go | 2 +- 9 files changed, 11 insertions(+), 11 deletions(-) diff --git a/http2/server_test.go b/http2/server_test.go index fd62dcb931..978cc37b46 100644 --- a/http2/server_test.go +++ b/http2/server_test.go @@ -1704,7 +1704,7 @@ func TestServer_Rejects_HeadersSelfDependence(t *testing.T) { }) } -// No PRIORTY frame with a self-dependence. +// No PRIORITY frame with a self-dependence. func TestServer_Rejects_PrioritySelfDependence(t *testing.T) { testServerRejectsStream(t, ErrCodeProtocol, func(st *serverTester) { st.fr.AllowIllegalWrites = true diff --git a/icmp/multipart.go b/icmp/multipart.go index 5f36675594..c7b72bf3dd 100644 --- a/icmp/multipart.go +++ b/icmp/multipart.go @@ -33,7 +33,7 @@ func multipartMessageBodyDataLen(proto int, withOrigDgram bool, b []byte, exts [ } // multipartMessageOrigDatagramLen takes b as an original datagram, -// and returns a required length for a padded orignal datagram in wire +// and returns a required length for a padded original datagram in wire // format. func multipartMessageOrigDatagramLen(proto int, b []byte) int { roundup := func(b []byte, align int) int { diff --git a/internal/sockstest/server.go b/internal/sockstest/server.go index dc2fa67c5e..c25a82f12a 100644 --- a/internal/sockstest/server.go +++ b/internal/sockstest/server.go @@ -46,7 +46,7 @@ func MarshalAuthReply(ver int, m socks.AuthMethod) ([]byte, error) { return []byte{byte(ver), byte(m)}, nil } -// A CmdRequest repesents a command request. +// A CmdRequest represents a command request. type CmdRequest struct { Version int Cmd socks.Command @@ -120,12 +120,12 @@ func MarshalCmdReply(ver int, reply socks.Reply, a *socks.Addr) ([]byte, error) return b, nil } -// A Server repesents a server for handshake testing. +// A Server represents a server for handshake testing. type Server struct { ln net.Listener } -// Addr rerurns a server address. +// Addr returns a server address. func (s *Server) Addr() net.Addr { return s.ln.Addr() } diff --git a/ipv4/multicastlistener_test.go b/ipv4/multicastlistener_test.go index 534ded6793..77bad6676c 100644 --- a/ipv4/multicastlistener_test.go +++ b/ipv4/multicastlistener_test.go @@ -142,7 +142,7 @@ func TestUDPPerInterfaceSinglePacketConnWithSingleGroupListener(t *testing.T) { } c, err := net.ListenPacket("udp4", net.JoinHostPort(ip.String(), port)) // unicast address with non-reusable port if err != nil { - // The listen may fail when the serivce is + // The listen may fail when the service is // already in use, but it's fine because the // purpose of this is not to test the // bookkeeping of IP control block inside the diff --git a/ipv6/dgramopt.go b/ipv6/dgramopt.go index 1f422e71dc..846f0e1f9c 100644 --- a/ipv6/dgramopt.go +++ b/ipv6/dgramopt.go @@ -245,7 +245,7 @@ func (c *dgramOpt) Checksum() (on bool, offset int, err error) { return true, offset, nil } -// SetChecksum enables the kernel checksum processing. If on is ture, +// SetChecksum enables the kernel checksum processing. If on is true, // the offset should be an offset in bytes into the data of where the // checksum field is located. func (c *dgramOpt) SetChecksum(on bool, offset int) error { diff --git a/ipv6/multicastlistener_test.go b/ipv6/multicastlistener_test.go index 353327e017..a4dc86342e 100644 --- a/ipv6/multicastlistener_test.go +++ b/ipv6/multicastlistener_test.go @@ -142,7 +142,7 @@ func TestUDPPerInterfaceSinglePacketConnWithSingleGroupListener(t *testing.T) { } c, err := net.ListenPacket("udp6", net.JoinHostPort(ip.String()+"%"+ifi.Name, port)) // unicast address with non-reusable port if err != nil { - // The listen may fail when the serivce is + // The listen may fail when the service is // already in use, but it's fine because the // purpose of this is not to test the // bookkeeping of IP control block inside the diff --git a/netutil/listen.go b/netutil/listen.go index d5dfbab24f..f8b779ea27 100644 --- a/netutil/listen.go +++ b/netutil/listen.go @@ -29,7 +29,7 @@ type limitListener struct { } // acquire acquires the limiting semaphore. Returns true if successfully -// accquired, false if the listener is closed and the semaphore is not +// acquired, false if the listener is closed and the semaphore is not // acquired. func (l *limitListener) acquire() bool { select { diff --git a/webdav/webdav.go b/webdav/webdav.go index 8d0f1b2aed..add2bcd67c 100644 --- a/webdav/webdav.go +++ b/webdav/webdav.go @@ -655,7 +655,7 @@ func handlePropfindError(err error, info os.FileInfo) error { // We need to be careful with other errors: there is no way to abort the xml stream // part way through while returning a valid PROPFIND response. Returning only half // the data would be misleading, but so would be returning results tainted by errors. - // The curent behaviour by returning an error here leads to the stream being aborted, + // The current behaviour by returning an error here leads to the stream being aborted, // and the parent http server complaining about writing a spurious header. We should // consider further enhancing this error handling to more gracefully fail, or perhaps // buffer the entire response until we've walked the tree. diff --git a/websocket/hybi.go b/websocket/hybi.go index 8cffdd16c9..48a069e190 100644 --- a/websocket/hybi.go +++ b/websocket/hybi.go @@ -369,7 +369,7 @@ func generateNonce() (nonce []byte) { return } -// removeZone removes IPv6 zone identifer from host. +// removeZone removes IPv6 zone identifier from host. // E.g., "[fe80::1%en0]:8080" to "[fe80::1]:8080" func removeZone(host string) string { if !strings.HasPrefix(host, "[") { From 415cb6d518e71d202e2dc2f44c475cbff84eee72 Mon Sep 17 00:00:00 2001 From: cui fliter Date: Tue, 7 Feb 2023 22:07:18 +0800 Subject: [PATCH 4/8] all: fix some comments Change-Id: Iee11c27052222f017b672c06ced9e129ee51619c Reviewed-on: https://go-review.googlesource.com/c/net/+/465996 Auto-Submit: Ian Lance Taylor Reviewed-by: Ian Lance Taylor Run-TryBot: Ian Lance Taylor Reviewed-by: David Chase TryBot-Result: Gopher Robot --- html/parse.go | 2 +- http2/flow.go | 2 +- http2/hpack/hpack.go | 2 +- http2/transport.go | 2 +- trace/histogram.go | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/html/parse.go b/html/parse.go index 291c91908d..46a89eda6c 100644 --- a/html/parse.go +++ b/html/parse.go @@ -184,7 +184,7 @@ func (p *parser) clearStackToContext(s scope) { } } -// parseGenericRawTextElements implements the generic raw text element parsing +// parseGenericRawTextElement implements the generic raw text element parsing // algorithm defined in 12.2.6.2. // https://html.spec.whatwg.org/multipage/parsing.html#parsing-elements-that-contain-only-text // TODO: Since both RAWTEXT and RCDATA states are treated as tokenizer's part diff --git a/http2/flow.go b/http2/flow.go index 750ac52f2a..b7dbd18695 100644 --- a/http2/flow.go +++ b/http2/flow.go @@ -18,7 +18,7 @@ type inflow struct { unsent int32 } -// set sets the initial window. +// init sets the initial window. func (f *inflow) init(n int32) { f.avail = n } diff --git a/http2/hpack/hpack.go b/http2/hpack/hpack.go index ebdfbee964..b184a2771a 100644 --- a/http2/hpack/hpack.go +++ b/http2/hpack/hpack.go @@ -211,7 +211,7 @@ func (d *Decoder) at(i uint64) (hf HeaderField, ok bool) { return dt.ents[dt.len()-(int(i)-staticTable.len())], true } -// Decode decodes an entire block. +// DecodeFull decodes an entire block. // // TODO: remove this method and make it incremental later? This is // easier for debugging now. diff --git a/http2/transport.go b/http2/transport.go index b43ec10cfe..05ba23d3d9 100644 --- a/http2/transport.go +++ b/http2/transport.go @@ -1569,7 +1569,7 @@ func (cs *clientStream) cleanupWriteRequest(err error) { close(cs.donec) } -// awaitOpenSlotForStream waits until len(streams) < maxConcurrentStreams. +// awaitOpenSlotForStreamLocked waits until len(streams) < maxConcurrentStreams. // Must hold cc.mu. func (cc *ClientConn) awaitOpenSlotForStreamLocked(cs *clientStream) error { for { diff --git a/trace/histogram.go b/trace/histogram.go index 9bf4286c79..d6c71101e4 100644 --- a/trace/histogram.go +++ b/trace/histogram.go @@ -32,7 +32,7 @@ type histogram struct { valueCount int64 // number of values recorded for single value } -// AddMeasurement records a value measurement observation to the histogram. +// addMeasurement records a value measurement observation to the histogram. func (h *histogram) addMeasurement(value int64) { // TODO: assert invariant h.sum += value From 87ce33ecb484cbb6bcfc8e506ce0330ef72e0847 Mon Sep 17 00:00:00 2001 From: Gopher Robot Date: Wed, 8 Feb 2023 18:10:08 +0000 Subject: [PATCH 5/8] go.mod: update golang.org/x dependencies Update golang.org/x dependencies to their latest tagged versions. Once this CL is submitted, and post-submit testing succeeds on all first-class ports across all supported Go versions, this repository will be tagged with its next minor version. Change-Id: Ia50695ab9c32268c5dfa5096e448c6d7cca851bd Reviewed-on: https://go-review.googlesource.com/c/net/+/466595 Reviewed-by: Heschi Kreinick TryBot-Result: Gopher Robot Auto-Submit: Gopher Robot Reviewed-by: Dmitri Shuralyov Run-TryBot: Gopher Robot --- go.mod | 6 +++--- go.sum | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 1b6c3b80d3..4d8f58967e 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module golang.org/x/net go 1.17 require ( - golang.org/x/sys v0.4.0 - golang.org/x/term v0.4.0 - golang.org/x/text v0.6.0 + golang.org/x/sys v0.5.0 + golang.org/x/term v0.5.0 + golang.org/x/text v0.7.0 ) diff --git a/go.sum b/go.sum index d530777939..bcd80060dd 100644 --- a/go.sum +++ b/go.sum @@ -12,17 +12,17 @@ golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.4.0 h1:Zr2JFtRQNX3BCZ8YtxRE9hNJYC8J6I1MVbMg6owUp18= -golang.org/x/sys v0.4.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.5.0 h1:MUK/U/4lj1t1oPg0HfuXDN/Z1wv31ZJ/YcPiGccS4DU= +golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= -golang.org/x/term v0.4.0 h1:O7UWfv5+A2qiuulQk30kVinPoMtoIPeVaKLEgLpVkvg= -golang.org/x/term v0.4.0/go.mod h1:9P2UbLfCdcvo3p/nzKvsmas4TnlujnuoV9hGgYzW1lQ= +golang.org/x/term v0.5.0 h1:n2a8QNdAb0sZNpU9R1ALUXBbY+w51fCQDN+7EdxNBsY= +golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= -golang.org/x/text v0.6.0 h1:3XmdazWV+ubf7QgHSTWeykHOci5oeekaGJBLkrkaw4k= -golang.org/x/text v0.6.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= +golang.org/x/text v0.7.0 h1:4BRB4x83lYWy72KwLD/qYDuTu7q9PjSagHvijDw7cLo= +golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= From 39940adcaaa73e661124cb80fb8dd57ea929dbaf Mon Sep 17 00:00:00 2001 From: Nigel Tao Date: Thu, 9 Feb 2023 10:57:03 +1100 Subject: [PATCH 6/8] html: parse comments per HTML spec Updates golang/go#58246 Change-Id: Iaba5ed65f5d244fd47372ef0c08fc4cdb5ed90f9 Reviewed-on: https://go-review.googlesource.com/c/net/+/466776 TryBot-Result: Gopher Robot Auto-Submit: Nigel Tao Reviewed-by: Damien Neil Run-TryBot: Nigel Tao Reviewed-by: Nigel Tao (INACTIVE; USE @golang.org INSTEAD) --- html/comment_test.go | 270 +++++++++++++++++++++++++++++++++++++++++++ html/token.go | 49 ++++++-- html/token_test.go | 37 +++++- 3 files changed, 347 insertions(+), 9 deletions(-) create mode 100644 html/comment_test.go diff --git a/html/comment_test.go b/html/comment_test.go new file mode 100644 index 0000000000..2c80bc748c --- /dev/null +++ b/html/comment_test.go @@ -0,0 +1,270 @@ +// Copyright 2023 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 html + +import ( + "bytes" + "testing" +) + +// TestComments exhaustively tests every 'interesting' N-byte string is +// correctly parsed as a comment. N ranges from 4+1 to 4+suffixLen inclusive, +// where 4 is the length of the "") return + } else if c == '-' { + dashCount = 1 + beginning = false + continue } } } @@ -645,6 +649,35 @@ func (z *Tokenizer) readComment() { } } +func (z *Tokenizer) calculateAbruptCommentDataEnd() int { + raw := z.Raw() + const prefixLen = len("", }, - // Comments. + // Comments. See also func TestComments. { "comment0", "abcdef", @@ -376,6 +376,41 @@ var tokenTests = []tokenTest{ "az", "a$$z", }, + { + "comment16", + "az", + "a$$z", + }, + { + "comment17", + "a", + }, + { + "comment18", + "az", + "a$$z", + }, + { + "comment19", + "a", + }, + { + "comment20", + "az", + "a$$z", + }, + { + "comment21", + "az", + "a$$z", + }, + { + "comment22", + "az", + "a$$z", + }, // An attribute with a backslash. { "backslash", From 547e7edf3873d6f3a9c093d3785f9e2289e00746 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 9 Feb 2023 11:20:23 -0800 Subject: [PATCH 7/8] http2: avoid referencing ResponseWrite.Write parameter after returning When writing data frames, encode the frame on the serve goroutine rather than in writeFrameAsync to avoid referencing stream data (originating from a ResponseWriter.Write call) after the Write has returned. Fixes golang/go#58446 Change-Id: I866a7351c90ef122e506b333151f98a455a64953 Reviewed-on: https://go-review.googlesource.com/c/net/+/467355 TryBot-Result: Gopher Robot Run-TryBot: Damien Neil Reviewed-by: Bryan Mills --- http2/frame.go | 11 ++++++- http2/server.go | 18 +++++++++-- http2/server_test.go | 75 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 4 deletions(-) diff --git a/http2/frame.go b/http2/frame.go index 184ac45feb..c1f6b90dc3 100644 --- a/http2/frame.go +++ b/http2/frame.go @@ -662,6 +662,15 @@ func (f *Framer) WriteData(streamID uint32, endStream bool, data []byte) error { // It is the caller's responsibility not to violate the maximum frame size // and to not call other Write methods concurrently. func (f *Framer) WriteDataPadded(streamID uint32, endStream bool, data, pad []byte) error { + if err := f.startWriteDataPadded(streamID, endStream, data, pad); err != nil { + return err + } + return f.endWrite() +} + +// startWriteDataPadded is WriteDataPadded, but only writes the frame to the Framer's internal buffer. +// The caller should call endWrite to flush the frame to the underlying writer. +func (f *Framer) startWriteDataPadded(streamID uint32, endStream bool, data, pad []byte) error { if !validStreamID(streamID) && !f.AllowIllegalWrites { return errStreamID } @@ -691,7 +700,7 @@ func (f *Framer) WriteDataPadded(streamID uint32, endStream bool, data, pad []by } f.wbuf = append(f.wbuf, data...) f.wbuf = append(f.wbuf, pad...) - return f.endWrite() + return nil } // A SettingsFrame conveys configuration parameters that affect how diff --git a/http2/server.go b/http2/server.go index 9bd7035bfe..8cb14f3c97 100644 --- a/http2/server.go +++ b/http2/server.go @@ -843,8 +843,13 @@ type frameWriteResult struct { // and then reports when it's done. // At most one goroutine can be running writeFrameAsync at a time per // serverConn. -func (sc *serverConn) writeFrameAsync(wr FrameWriteRequest) { - err := wr.write.writeFrame(sc) +func (sc *serverConn) writeFrameAsync(wr FrameWriteRequest, wd *writeData) { + var err error + if wd == nil { + err = wr.write.writeFrame(sc) + } else { + err = sc.framer.endWrite() + } sc.wroteFrameCh <- frameWriteResult{wr: wr, err: err} } @@ -1251,9 +1256,16 @@ func (sc *serverConn) startFrameWrite(wr FrameWriteRequest) { sc.writingFrameAsync = false err := wr.write.writeFrame(sc) sc.wroteFrame(frameWriteResult{wr: wr, err: err}) + } else if wd, ok := wr.write.(*writeData); ok { + // Encode the frame in the serve goroutine, to ensure we don't have + // any lingering asynchronous references to data passed to Write. + // See https://go.dev/issue/58446. + sc.framer.startWriteDataPadded(wd.streamID, wd.endStream, wd.p, nil) + sc.writingFrameAsync = true + go sc.writeFrameAsync(wr, wd) } else { sc.writingFrameAsync = true - go sc.writeFrameAsync(wr) + go sc.writeFrameAsync(wr, nil) } } diff --git a/http2/server_test.go b/http2/server_test.go index 978cc37b46..d32b2d85bd 100644 --- a/http2/server_test.go +++ b/http2/server_test.go @@ -4631,3 +4631,78 @@ func TestCanonicalHeaderCacheGrowth(t *testing.T) { } } } + +// TestServerWriteDoesNotRetainBufferAfterStreamClose checks for access to +// the slice passed to ResponseWriter.Write after Write returns. +// +// Terminating the request stream on the client causes Write to return. +// We should not access the slice after this point. +func TestServerWriteDoesNotRetainBufferAfterReturn(t *testing.T) { + donec := make(chan struct{}) + st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) { + defer close(donec) + buf := make([]byte, 1<<20) + var i byte + for { + i++ + _, err := w.Write(buf) + for j := range buf { + buf[j] = byte(i) // trigger race detector + } + if err != nil { + return + } + } + }, optOnlyServer) + defer st.Close() + + tr := &Transport{TLSClientConfig: tlsConfigInsecure} + defer tr.CloseIdleConnections() + + req, _ := http.NewRequest("GET", st.ts.URL, nil) + res, err := tr.RoundTrip(req) + if err != nil { + t.Fatal(err) + } + res.Body.Close() + <-donec +} + +// TestServerWriteDoesNotRetainBufferAfterServerClose checks for access to +// the slice passed to ResponseWriter.Write after Write returns. +// +// Shutting down the Server causes Write to return. +// We should not access the slice after this point. +func TestServerWriteDoesNotRetainBufferAfterServerClose(t *testing.T) { + donec := make(chan struct{}, 1) + st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) { + donec <- struct{}{} + defer close(donec) + buf := make([]byte, 1<<20) + var i byte + for { + i++ + _, err := w.Write(buf) + for j := range buf { + buf[j] = byte(i) + } + if err != nil { + return + } + } + }, optOnlyServer) + defer st.Close() + + tr := &Transport{TLSClientConfig: tlsConfigInsecure} + defer tr.CloseIdleConnections() + + req, _ := http.NewRequest("GET", st.ts.URL, nil) + res, err := tr.RoundTrip(req) + if err != nil { + t.Fatal(err) + } + defer res.Body.Close() + <-donec + st.ts.Config.Close() + <-donec +} From 8e2b117aee74f6b86c207a808b0255de45c0a18a Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 22 Dec 2022 09:33:10 -0800 Subject: [PATCH 8/8] http2/hpack: avoid quadratic complexity in hpack decoding When parsing a field literal containing two Huffman-encoded strings, don't decode the first string until verifying all data is present. Avoids forced quadratic complexity when repeatedly parsing a partial field, repeating the Huffman decoding of the string on each iteration. Thanks to Philippe Antoine (Catena cyber) for reporting this issue. Fixes golang/go#57855 Fixes CVE-2022-41723 Change-Id: I58a743df450a4a4923dddd5cf6bb0592b0a7bdf3 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1688184 TryBot-Result: Security TryBots Reviewed-by: Julie Qiu Run-TryBot: Damien Neil Reviewed-by: Roland Shoemaker Reviewed-on: https://go-review.googlesource.com/c/net/+/468135 Run-TryBot: Michael Pratt Reviewed-by: Roland Shoemaker Reviewed-by: Than McIntosh Auto-Submit: Michael Pratt TryBot-Result: Gopher Robot --- http2/hpack/hpack.go | 79 ++++++++++++++++++++++++--------------- http2/hpack/hpack_test.go | 30 +++++++++++++++ 2 files changed, 79 insertions(+), 30 deletions(-) diff --git a/http2/hpack/hpack.go b/http2/hpack/hpack.go index b184a2771a..7a1d976696 100644 --- a/http2/hpack/hpack.go +++ b/http2/hpack/hpack.go @@ -359,6 +359,7 @@ func (d *Decoder) parseFieldLiteral(n uint8, it indexType) error { var hf HeaderField wantStr := d.emitEnabled || it.indexed() + var undecodedName undecodedString if nameIdx > 0 { ihf, ok := d.at(nameIdx) if !ok { @@ -366,15 +367,27 @@ func (d *Decoder) parseFieldLiteral(n uint8, it indexType) error { } hf.Name = ihf.Name } else { - hf.Name, buf, err = d.readString(buf, wantStr) + undecodedName, buf, err = d.readString(buf) if err != nil { return err } } - hf.Value, buf, err = d.readString(buf, wantStr) + undecodedValue, buf, err := d.readString(buf) if err != nil { return err } + if wantStr { + if nameIdx <= 0 { + hf.Name, err = d.decodeString(undecodedName) + if err != nil { + return err + } + } + hf.Value, err = d.decodeString(undecodedValue) + if err != nil { + return err + } + } d.buf = buf if it.indexed() { d.dynTab.add(hf) @@ -459,46 +472,52 @@ func readVarInt(n byte, p []byte) (i uint64, remain []byte, err error) { return 0, origP, errNeedMore } -// readString decodes an hpack string from p. +// readString reads an hpack string from p. // -// wantStr is whether s will be used. If false, decompression and -// []byte->string garbage are skipped if s will be ignored -// anyway. This does mean that huffman decoding errors for non-indexed -// strings past the MAX_HEADER_LIST_SIZE are ignored, but the server -// is returning an error anyway, and because they're not indexed, the error -// won't affect the decoding state. -func (d *Decoder) readString(p []byte, wantStr bool) (s string, remain []byte, err error) { +// It returns a reference to the encoded string data to permit deferring decode costs +// until after the caller verifies all data is present. +func (d *Decoder) readString(p []byte) (u undecodedString, remain []byte, err error) { if len(p) == 0 { - return "", p, errNeedMore + return u, p, errNeedMore } isHuff := p[0]&128 != 0 strLen, p, err := readVarInt(7, p) if err != nil { - return "", p, err + return u, p, err } if d.maxStrLen != 0 && strLen > uint64(d.maxStrLen) { - return "", nil, ErrStringLength + // Returning an error here means Huffman decoding errors + // for non-indexed strings past the maximum string length + // are ignored, but the server is returning an error anyway + // and because the string is not indexed the error will not + // affect the decoding state. + return u, nil, ErrStringLength } if uint64(len(p)) < strLen { - return "", p, errNeedMore - } - if !isHuff { - if wantStr { - s = string(p[:strLen]) - } - return s, p[strLen:], nil + return u, p, errNeedMore } + u.isHuff = isHuff + u.b = p[:strLen] + return u, p[strLen:], nil +} - if wantStr { - buf := bufPool.Get().(*bytes.Buffer) - buf.Reset() // don't trust others - defer bufPool.Put(buf) - if err := huffmanDecode(buf, d.maxStrLen, p[:strLen]); err != nil { - buf.Reset() - return "", nil, err - } +type undecodedString struct { + isHuff bool + b []byte +} + +func (d *Decoder) decodeString(u undecodedString) (string, error) { + if !u.isHuff { + return string(u.b), nil + } + buf := bufPool.Get().(*bytes.Buffer) + buf.Reset() // don't trust others + var s string + err := huffmanDecode(buf, d.maxStrLen, u.b) + if err == nil { s = buf.String() - buf.Reset() // be nice to GC } - return s, p[strLen:], nil + buf.Reset() // be nice to GC + bufPool.Put(buf) + return s, err } diff --git a/http2/hpack/hpack_test.go b/http2/hpack/hpack_test.go index f9092e8bb9..b4b2a5d666 100644 --- a/http2/hpack/hpack_test.go +++ b/http2/hpack/hpack_test.go @@ -728,6 +728,36 @@ func TestEmitEnabled(t *testing.T) { } } +func TestSlowIncrementalDecode(t *testing.T) { + // TODO(dneil): Fix for -race mode. + t.Skip("too slow in -race mode") + + var buf bytes.Buffer + enc := NewEncoder(&buf) + hf := HeaderField{ + Name: strings.Repeat("k", 1<<20), + Value: strings.Repeat("v", 1<<20), + } + enc.WriteField(hf) + hbuf := buf.Bytes() + count := 0 + dec := NewDecoder(initialHeaderTableSize, func(got HeaderField) { + count++ + if count != 1 { + t.Errorf("decoded %v fields, want 1", count) + } + if got.Name != hf.Name { + t.Errorf("decoded Name does not match input") + } + if got.Value != hf.Value { + t.Errorf("decoded Value does not match input") + } + }) + for i := 0; i < len(hbuf); i++ { + dec.Write(hbuf[i : i+1]) + } +} + func TestSaveBufLimit(t *testing.T) { const maxStr = 1 << 10 var got []HeaderField