From 26b646ea024741dd5d8e141fc33d8149c465686a Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Mon, 8 Jan 2024 09:36:16 -0800 Subject: [PATCH 1/7] quic: avoid deadlock in Endpoint.Close Don't hold Endpoint.connsMu while calling Conn methods that can indirectly depend on acquiring it. Also change test cleanup to not wait for connections to drain when closing a test Endpoint, removing an unnecessary 0.1s delay in test runtime. Fixes golang/go#64982. Change-Id: If336e63b0a7f5b8d2ef63986d36f9ee38a92c477 Reviewed-on: https://go-review.googlesource.com/c/net/+/554695 Reviewed-by: Jonathan Amsterdam LUCI-TryBot-Result: Go LUCI --- internal/quic/endpoint.go | 16 +++++++++++----- internal/quic/endpoint_test.go | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/quic/endpoint.go b/internal/quic/endpoint.go index 82a08a18c..8ed67de54 100644 --- a/internal/quic/endpoint.go +++ b/internal/quic/endpoint.go @@ -103,25 +103,31 @@ func (e *Endpoint) LocalAddr() netip.AddrPort { // It waits for the peers of any open connection to acknowledge the connection has been closed. func (e *Endpoint) Close(ctx context.Context) error { e.acceptQueue.close(errors.New("endpoint closed")) + + // It isn't safe to call Conn.Abort or conn.exit with connsMu held, + // so copy the list of conns. + var conns []*Conn e.connsMu.Lock() if !e.closing { - e.closing = true + e.closing = true // setting e.closing prevents new conns from being created for c := range e.conns { - c.Abort(localTransportError{code: errNo}) + conns = append(conns, c) } if len(e.conns) == 0 { e.udpConn.Close() } } e.connsMu.Unlock() + + for _, c := range conns { + c.Abort(localTransportError{code: errNo}) + } select { case <-e.closec: case <-ctx.Done(): - e.connsMu.Lock() - for c := range e.conns { + for _, c := range conns { c.exit() } - e.connsMu.Unlock() return ctx.Err() } return nil diff --git a/internal/quic/endpoint_test.go b/internal/quic/endpoint_test.go index ab6cd1cf5..16c3e0bce 100644 --- a/internal/quic/endpoint_test.go +++ b/internal/quic/endpoint_test.go @@ -97,7 +97,7 @@ func newLocalEndpoint(t *testing.T, side connSide, conf *Config) *Endpoint { t.Fatal(err) } t.Cleanup(func() { - e.Close(context.Background()) + e.Close(canceledContext()) }) return e } From 07e05fd6e95ab445ebe48840c81a027dbace3b8e Mon Sep 17 00:00:00 2001 From: Nikita Sivukhin Date: Fri, 5 Jan 2024 11:25:21 +0400 Subject: [PATCH 2/7] http2: remove suspicious uint32->v conversion in frame code Function maxHeaderStringLen(...) uses uint32(int(v)) == v check to validate if length will fit in the int type. This check is a no-op on any architecture because int type always has at least 32 bits, so we can potentially encounter negative return values from maxHeaderStringLen(...) function. This can be bad as this outcome clearly breaks code intention and maybe some further code invariants. This patch replaces uint32(int(v)) == v check with more robust and simpler int(v) > 0 validation which is correct for our case when we operating with uint32 Fixes golang/go#64961 Change-Id: I31f95709df9d25593ade3200696ac5cef9f88652 Reviewed-on: https://go-review.googlesource.com/c/net/+/554235 Auto-Submit: Dmitri Shuralyov Reviewed-by: Damien Neil LUCI-TryBot-Result: Go LUCI Reviewed-by: Dmitri Shuralyov --- http2/frame.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/http2/frame.go b/http2/frame.go index c1f6b90dc..e2b298d85 100644 --- a/http2/frame.go +++ b/http2/frame.go @@ -1510,13 +1510,12 @@ func (mh *MetaHeadersFrame) checkPseudos() error { } func (fr *Framer) maxHeaderStringLen() int { - v := fr.maxHeaderListSize() - if uint32(int(v)) == v { - return int(v) + v := int(fr.maxHeaderListSize()) + if v < 0 { + // If maxHeaderListSize overflows an int, use no limit (0). + return 0 } - // They had a crazy big number for MaxHeaderBytes anyway, - // so give them unlimited header lengths: - return 0 + return v } // readMetaFrame returns 0 or more CONTINUATION frames from fr and From 0d0b98c1378dba60d10c77c383c40f94c1641cfc Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Mon, 22 Jan 2024 16:26:00 -0500 Subject: [PATCH 3/7] http2: avoid goroutine starvation in TestServer_Push_RejectAfterGoAway CL 557037 added a runtime.Gosched to prevent goroutine starvation in the wasm fake-net stack. Unfortunately, that Gosched causes the scheduler to enter a very similar starvation loop in this test. Add another runtime.Gosched to break this new loop. For golang/go#65178. Change-Id: I24b3f50dd728800462f71f27290b0d8f99d5ae5b Cq-Include-Trybots: luci.golang.try:x_net-gotip-wasip1-wasm_wasmtime,x_net-gotip-wasip1-wasm_wazero,x_net-gotip-js-wasm Reviewed-on: https://go-review.googlesource.com/c/net/+/557615 Auto-Submit: Bryan Mills LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Pratt --- http2/server_push_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/http2/server_push_test.go b/http2/server_push_test.go index 9882d9ef7..cda8f4336 100644 --- a/http2/server_push_test.go +++ b/http2/server_push_test.go @@ -11,6 +11,7 @@ import ( "io/ioutil" "net/http" "reflect" + "runtime" "strconv" "sync" "testing" @@ -483,11 +484,7 @@ func TestServer_Push_RejectAfterGoAway(t *testing.T) { ready := make(chan struct{}) errc := make(chan error, 2) st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) { - select { - case <-ready: - case <-time.After(5 * time.Second): - errc <- fmt.Errorf("timeout waiting for GOAWAY to be processed") - } + <-ready if got, want := w.(http.Pusher).Push("https://"+r.Host+"/pushed", nil), http.ErrNotSupported; got != want { errc <- fmt.Errorf("Push()=%v, want %v", got, want) } @@ -505,6 +502,10 @@ func TestServer_Push_RejectAfterGoAway(t *testing.T) { case <-ready: return default: + if runtime.GOARCH == "wasm" { + // Work around https://go.dev/issue/65178 to avoid goroutine starvation. + runtime.Gosched() + } } st.sc.serveMsgCh <- func(loopNum int) { if !st.sc.pushEnabled { From b2208d046df5625a4f78624149cba7722c4ccfee Mon Sep 17 00:00:00 2001 From: btwiuse <54848194+btwiuse@users.noreply.github.com> Date: Sun, 14 Jan 2024 13:20:58 +0000 Subject: [PATCH 4/7] internal/quic/qlog: fix typo VantageClient -> VantageServer Change-Id: Ie9738cffb06f03f961815853247e6f9cbe7fe466 GitHub-Last-Rev: 5d440ad29c49ef4cd529a076449114696662afec GitHub-Pull-Request: golang/net#202 Reviewed-on: https://go-review.googlesource.com/c/net/+/555795 LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Knyszek Reviewed-by: Damien Neil Auto-Submit: Damien Neil --- internal/quic/qlog/qlog.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/quic/qlog/qlog.go b/internal/quic/qlog/qlog.go index e54c839f0..f33c6b0fd 100644 --- a/internal/quic/qlog/qlog.go +++ b/internal/quic/qlog/qlog.go @@ -29,7 +29,7 @@ const ( // VantageClient traces follow a connection from the client's perspective. VantageClient = Vantage("client") - // VantageClient traces follow a connection from the server's perspective. + // VantageServer traces follow a connection from the server's perspective. VantageServer = Vantage("server") ) From 73e4b50dadcf3bd6015efb8b6e8ddbeb7dfe74c5 Mon Sep 17 00:00:00 2001 From: Mateusz Poliwczak <19653795+mateusz834@users.noreply.github.com> Date: Fri, 2 Feb 2024 15:48:44 +0000 Subject: [PATCH 5/7] dns/dnsmessage: allow name compression for SRV resource parsing As per RFC 3597: Receiving servers MUST decompress domain names in RRs of well-known type, and SHOULD also decompress RRs of type RP, AFSDB, RT, SIG, PX, NXT, NAPTR, and SRV (although the current specification of the SRV RR in RFC2782 prohibits compression, RFC2052 mandated it, and some servers following that earlier specification are still in use). This change allows SRV resource decompression. Updates golang/go#36718 Updates golang/go#37362 Change-Id: I473c0d3803758e5b12886f378d2ed54bd5392144 GitHub-Last-Rev: 88d2e0642a7c7ba618d642801ebc72ba82ef30b7 GitHub-Pull-Request: golang/net#199 Reviewed-on: https://go-review.googlesource.com/c/net/+/540375 LUCI-TryBot-Result: Go LUCI Reviewed-by: Carlos Amedee Auto-Submit: Damien Neil Reviewed-by: Damien Neil --- dns/dnsmessage/message.go | 10 +--------- dns/dnsmessage/message_test.go | 22 ---------------------- 2 files changed, 1 insertion(+), 31 deletions(-) diff --git a/dns/dnsmessage/message.go b/dns/dnsmessage/message.go index 42987ab7c..a656efc12 100644 --- a/dns/dnsmessage/message.go +++ b/dns/dnsmessage/message.go @@ -273,7 +273,6 @@ var ( errTooManyAdditionals = errors.New("too many Additionals to pack (>65535)") errNonCanonicalName = errors.New("name is not in canonical format (it must end with a .)") errStringTooLong = errors.New("character string exceeds maximum length (255)") - errCompressedSRV = errors.New("compressed name in SRV resource data") ) // Internal constants. @@ -2028,10 +2027,6 @@ func (n *Name) pack(msg []byte, compression map[string]uint16, compressionOff in // unpack unpacks a domain name. func (n *Name) unpack(msg []byte, off int) (int, error) { - return n.unpackCompressed(msg, off, true /* allowCompression */) -} - -func (n *Name) unpackCompressed(msg []byte, off int, allowCompression bool) (int, error) { // currOff is the current working offset. currOff := off @@ -2076,9 +2071,6 @@ Loop: name = append(name, '.') currOff = endOff case 0xC0: // Pointer - if !allowCompression { - return off, errCompressedSRV - } if currOff >= len(msg) { return off, errInvalidPtr } @@ -2549,7 +2541,7 @@ func unpackSRVResource(msg []byte, off int) (SRVResource, error) { return SRVResource{}, &nestedError{"Port", err} } var target Name - if _, err := target.unpackCompressed(msg, off, false /* allowCompression */); err != nil { + if _, err := target.unpack(msg, off); err != nil { return SRVResource{}, &nestedError{"Target", err} } return SRVResource{priority, weight, port, target}, nil diff --git a/dns/dnsmessage/message_test.go b/dns/dnsmessage/message_test.go index c84d5a3aa..e60ec42d9 100644 --- a/dns/dnsmessage/message_test.go +++ b/dns/dnsmessage/message_test.go @@ -303,28 +303,6 @@ func TestNameUnpackTooLongName(t *testing.T) { } } -func TestIncompressibleName(t *testing.T) { - name := MustNewName("example.com.") - compression := map[string]uint16{} - buf, err := name.pack(make([]byte, 0, 100), compression, 0) - if err != nil { - t.Fatal("first Name.pack() =", err) - } - buf, err = name.pack(buf, compression, 0) - if err != nil { - t.Fatal("second Name.pack() =", err) - } - var n1 Name - off, err := n1.unpackCompressed(buf, 0, false /* allowCompression */) - if err != nil { - t.Fatal("unpacking incompressible name without pointers failed:", err) - } - var n2 Name - if _, err := n2.unpackCompressed(buf, off, false /* allowCompression */); err != errCompressedSRV { - t.Errorf("unpacking compressed incompressible name with pointers: got %v, want = %v", err, errCompressedSRV) - } -} - func checkErrorPrefix(err error, prefix string) bool { e, ok := err.(*nestedError) return ok && e.s == prefix From 643fd162e36ae58085b92ff4c0fec0bafe5a46a7 Mon Sep 17 00:00:00 2001 From: Maciej Mionskowski Date: Thu, 19 Oct 2023 20:16:20 +0000 Subject: [PATCH 6/7] html: fix SOLIDUS '/' handling in attribute parsing Calling the Tokenizer with HTML elements containing SOLIDUS (/) character in the attribute name results in incorrect tokenization. This is due to violation of the following rule transitions in the WHATWG spec: - https://html.spec.whatwg.org/multipage/parsing.html#attribute-name-state, where we are not reconsuming the character if '/' is encountered - https://html.spec.whatwg.org/multipage/parsing.html#after-attribute-name-state, where we are not switching to self closing state Fixes golang/go#63402 Change-Id: I90d998dd8decde877bd63aa664f3657aa6161024 GitHub-Last-Rev: 3546db808c5fbf46ea25a10cdadb2802f763b6de GitHub-Pull-Request: golang/net#195 Reviewed-on: https://go-review.googlesource.com/c/net/+/533518 LUCI-TryBot-Result: Go LUCI Auto-Submit: Michael Pratt Reviewed-by: Roland Shoemaker Reviewed-by: David Chase --- html/token.go | 12 ++++++++---- html/token_test.go | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/html/token.go b/html/token.go index de67f938a..3c57880d6 100644 --- a/html/token.go +++ b/html/token.go @@ -910,9 +910,6 @@ func (z *Tokenizer) readTagAttrKey() { return } switch c { - case ' ', '\n', '\r', '\t', '\f', '/': - z.pendingAttr[0].end = z.raw.end - 1 - return case '=': if z.pendingAttr[0].start+1 == z.raw.end { // WHATWG 13.2.5.32, if we see an equals sign before the attribute name @@ -920,7 +917,9 @@ func (z *Tokenizer) readTagAttrKey() { continue } fallthrough - case '>': + case ' ', '\n', '\r', '\t', '\f', '/', '>': + // WHATWG 13.2.5.33 Attribute name state + // We need to reconsume the char in the after attribute name state to support the / character z.raw.end-- z.pendingAttr[0].end = z.raw.end return @@ -939,6 +938,11 @@ func (z *Tokenizer) readTagAttrVal() { if z.err != nil { return } + if c == '/' { + // WHATWG 13.2.5.34 After attribute name state + // U+002F SOLIDUS (/) - Switch to the self-closing start tag state. + return + } if c != '=' { z.raw.end-- return diff --git a/html/token_test.go b/html/token_test.go index b2383a951..8b0d5aab6 100644 --- a/html/token_test.go +++ b/html/token_test.go @@ -601,6 +601,21 @@ var tokenTests = []tokenTest{ `

`, `

`, }, + { + "forward slash before attribute name", + `

`, + `

`, + }, + { + "forward slash before attribute name with spaces around", + `

`, + `

`, + }, + { + "forward slash after attribute name followed by a character", + `

`, + `

`, + }, } func TestTokenizer(t *testing.T) { From 73d21fdbb4d7dc7115b50526b93b6c37a4e3377f Mon Sep 17 00:00:00 2001 From: Gopher Robot Date: Wed, 7 Feb 2024 19:22:03 +0000 Subject: [PATCH 7/7] go.mod: update golang.org/x dependencies Update golang.org/x dependencies to their latest tagged versions. Change-Id: I314af161ceac84fec04c729a71860ad35335513b Reviewed-on: https://go-review.googlesource.com/c/net/+/562495 Auto-Submit: Gopher Robot Reviewed-by: Dmitri Shuralyov LUCI-TryBot-Result: Go LUCI Reviewed-by: Than McIntosh --- go.mod | 6 +++--- go.sum | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 3bd487f5a..7f512d703 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,8 @@ module golang.org/x/net go 1.18 require ( - golang.org/x/crypto v0.18.0 - golang.org/x/sys v0.16.0 - golang.org/x/term v0.16.0 + golang.org/x/crypto v0.19.0 + golang.org/x/sys v0.17.0 + golang.org/x/term v0.17.0 golang.org/x/text v0.14.0 ) diff --git a/go.sum b/go.sum index 8eeaf16c6..683b469d6 100644 --- a/go.sum +++ b/go.sum @@ -1,8 +1,8 @@ -golang.org/x/crypto v0.18.0 h1:PGVlW0xEltQnzFZ55hkuX5+KLyrMYhHld1YHO4AKcdc= -golang.org/x/crypto v0.18.0/go.mod h1:R0j02AL6hcrfOiy9T4ZYp/rcWeMxM3L6QYxlOuEG1mg= -golang.org/x/sys v0.16.0 h1:xWw16ngr6ZMtmxDyKyIgsE93KNKz5HKmMa3b8ALHidU= -golang.org/x/sys v0.16.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/term v0.16.0 h1:m+B6fahuftsE9qjo0VWp2FW0mB3MTJvR0BaMQrq0pmE= -golang.org/x/term v0.16.0/go.mod h1:yn7UURbUtPyrVJPGPq404EukNFxcm/foM+bV/bfcDsY= +golang.org/x/crypto v0.19.0 h1:ENy+Az/9Y1vSrlrvBSyna3PITt4tiZLf7sgCjZBX7Wo= +golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= +golang.org/x/sys v0.17.0 h1:25cE3gD+tdBA7lp7QfhuV+rJiE9YXTcS3VG1SqssI/Y= +golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/term v0.17.0 h1:mkTF7LCd6WGJNL3K1Ad7kwxNfYAW6a8a8QqtMblp/4U= +golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU=