From 0ef1b2f92e4f7637ddb96b8e4a1c095f26526198 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 10 Jul 2020 11:21:12 +0700 Subject: [PATCH] pass around the stateless reset token directly, not pointers to it Benchmarks show that it's actually faster to make a copy of this 16 byte array than passing around a pointer to it. --- conn_id_manager.go | 12 ++++++------ conn_id_manager_test.go | 16 +++++++--------- internal/utils/new_connection_id.go | 2 +- packet_handler_map.go | 6 +++--- packet_handler_map_test.go | 4 ++-- session.go | 4 ++-- session_test.go | 2 +- 7 files changed, 22 insertions(+), 24 deletions(-) diff --git a/conn_id_manager.go b/conn_id_manager.go index 92bffe20..46b0d115 100644 --- a/conn_id_manager.go +++ b/conn_id_manager.go @@ -53,7 +53,7 @@ func newConnIDManager( } } -func (h *connIDManager) AddFromPreferredAddress(connID protocol.ConnectionID, resetToken *[16]byte) error { +func (h *connIDManager) AddFromPreferredAddress(connID protocol.ConnectionID, resetToken [16]byte) error { return h.addConnectionID(1, connID, resetToken) } @@ -98,7 +98,7 @@ func (h *connIDManager) add(f *wire.NewConnectionIDFrame) error { return nil } - if err := h.addConnectionID(f.SequenceNumber, f.ConnectionID, &f.StatelessResetToken); err != nil { + if err := h.addConnectionID(f.SequenceNumber, f.ConnectionID, f.StatelessResetToken); err != nil { return err } @@ -110,7 +110,7 @@ func (h *connIDManager) add(f *wire.NewConnectionIDFrame) error { return nil } -func (h *connIDManager) addConnectionID(seq uint64, connID protocol.ConnectionID, resetToken *[16]byte) error { +func (h *connIDManager) addConnectionID(seq uint64, connID protocol.ConnectionID, resetToken [16]byte) error { // insert a new element at the end if h.queue.Len() == 0 || h.queue.Back().Value.SequenceNumber < seq { h.queue.PushBack(utils.NewConnectionID{ @@ -126,7 +126,7 @@ func (h *connIDManager) addConnectionID(seq uint64, connID protocol.ConnectionID if !el.Value.ConnectionID.Equal(connID) { return fmt.Errorf("received conflicting connection IDs for sequence number %d", seq) } - if *el.Value.StatelessResetToken != *resetToken { + if el.Value.StatelessResetToken != resetToken { return fmt.Errorf("received conflicting stateless reset tokens for sequence number %d", seq) } break @@ -155,7 +155,7 @@ func (h *connIDManager) updateConnectionID() { front := h.queue.Remove(h.queue.Front()) h.activeSequenceNumber = front.SequenceNumber h.activeConnectionID = front.ConnectionID - h.activeStatelessResetToken = front.StatelessResetToken + h.activeStatelessResetToken = &front.StatelessResetToken h.packetsSinceLastChange = 0 h.packetsPerConnectionID = protocol.PacketsPerConnectionID/2 + uint64(h.rand.Int63n(protocol.PacketsPerConnectionID)) h.addStatelessResetToken(*h.activeStatelessResetToken) @@ -190,7 +190,7 @@ func (h *connIDManager) SentPacket() { } func (h *connIDManager) shouldUpdateConnID() bool { - // iniate the first change as early as possible + // initiate the first change as early as possible if h.queue.Len() > 0 && h.activeSequenceNumber == 0 { return true } diff --git a/conn_id_manager_test.go b/conn_id_manager_test.go index 34243f6f..6287fbde 100644 --- a/conn_id_manager_test.go +++ b/conn_id_manager_test.go @@ -33,9 +33,9 @@ var _ = Describe("Connection ID Manager", func() { }) }) - get := func() (protocol.ConnectionID, *[16]byte) { + get := func() (protocol.ConnectionID, [16]byte) { if m.queue.Len() == 0 { - return nil, nil + return nil, [16]byte{} } val := m.queue.Remove(m.queue.Front()) return val.ConnectionID, val.StatelessResetToken @@ -70,13 +70,12 @@ var _ = Describe("Connection ID Manager", func() { })).To(Succeed()) c1, rt1 := get() Expect(c1).To(Equal(protocol.ConnectionID{1, 2, 3, 4})) - Expect(*rt1).To(Equal([16]byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe})) + Expect(rt1).To(Equal([16]byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe})) c2, rt2 := get() Expect(c2).To(Equal(protocol.ConnectionID{2, 3, 4, 5})) - Expect(*rt2).To(Equal([16]byte{0xe, 0xd, 0xc, 0xb, 0xa, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0})) - c3, rt3 := get() + Expect(rt2).To(Equal([16]byte{0xe, 0xd, 0xc, 0xb, 0xa, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0})) + c3, _ := get() Expect(c3).To(BeNil()) - Expect(rt3).To(BeNil()) }) It("accepts duplicates", func() { @@ -94,10 +93,9 @@ var _ = Describe("Connection ID Manager", func() { Expect(m.Add(f2)).To(Succeed()) c1, rt1 := get() Expect(c1).To(Equal(protocol.ConnectionID{1, 2, 3, 4})) - Expect(*rt1).To(Equal([16]byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe})) - c2, rt2 := get() + Expect(rt1).To(Equal([16]byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe})) + c2, _ := get() Expect(c2).To(BeNil()) - Expect(rt2).To(BeNil()) }) It("ignores duplicates for the currently used connection ID", func() { diff --git a/internal/utils/new_connection_id.go b/internal/utils/new_connection_id.go index dd0d8bda..54ab0149 100644 --- a/internal/utils/new_connection_id.go +++ b/internal/utils/new_connection_id.go @@ -8,5 +8,5 @@ import ( type NewConnectionID struct { SequenceNumber uint64 ConnectionID protocol.ConnectionID - StatelessResetToken *[16]byte + StatelessResetToken [16]byte } diff --git a/packet_handler_map.go b/packet_handler_map.go index 49195c3a..041ed4f3 100644 --- a/packet_handler_map.go +++ b/packet_handler_map.go @@ -16,11 +16,11 @@ import ( ) type statelessResetErr struct { - token *[16]byte + token [16]byte } func (e statelessResetErr) Error() string { - return fmt.Sprintf("received a stateless reset with token %x", *e.token) + return fmt.Sprintf("received a stateless reset with token %x", e.token) } // The packetHandlerMap stores packetHandlers, identified by connection ID. @@ -317,7 +317,7 @@ func (h *packetHandlerMap) maybeHandleStatelessReset(data []byte) bool { copy(token[:], data[len(data)-16:]) if sess, ok := h.resetTokens[token]; ok { h.logger.Debugf("Received a stateless reset with token %#x. Closing session.", token) - go sess.destroy(statelessResetErr{token: &token}) + go sess.destroy(statelessResetErr{token: token}) return true } return false diff --git a/packet_handler_map_test.go b/packet_handler_map_test.go index c08c75ac..0f51d1d0 100644 --- a/packet_handler_map_test.go +++ b/packet_handler_map_test.go @@ -247,7 +247,7 @@ var _ = Describe("Packet Handler Map", func() { var resetErr statelessResetErr Expect(errors.As(err, &resetErr)).To(BeTrue()) Expect(err.Error()).To(ContainSubstring("received a stateless reset")) - Expect(resetErr.token).To(Equal(&token)) + Expect(resetErr.token).To(Equal(token)) close(destroyed) }) conn.dataToRead <- packet @@ -268,7 +268,7 @@ var _ = Describe("Packet Handler Map", func() { var resetErr statelessResetErr Expect(errors.As(err, &resetErr)).To(BeTrue()) Expect(err.Error()).To(ContainSubstring("received a stateless reset")) - Expect(resetErr.token).To(Equal(&token)) + Expect(resetErr.token).To(Equal(token)) close(destroyed) }) conn.dataToRead <- packet diff --git a/session.go b/session.go index 09fe0e10..d37da7f8 100644 --- a/session.go +++ b/session.go @@ -1301,7 +1301,7 @@ func (s *session) handleCloseError(closeErr closeError) { if nerr, ok := closeErr.err.(net.Error); !ok || !nerr.Timeout() { var resetErr statelessResetErr if errors.As(closeErr.err, &resetErr) { - s.tracer.ClosedConnection(logging.NewStatelessResetCloseReason(resetErr.token)) + s.tracer.ClosedConnection(logging.NewStatelessResetCloseReason(&resetErr.token)) } else if quicErr.IsApplicationError() { s.tracer.ClosedConnection(logging.NewApplicationCloseReason(quicErr.ErrorCode, closeErr.remote)) } else { @@ -1408,7 +1408,7 @@ func (s *session) processTransportParametersImpl(params *wire.TransportParameter if params.PreferredAddress != nil { s.logger.Debugf("Server sent preferred_address. Retiring the preferred_address connection ID.") // Retire the connection ID. - s.connIDManager.AddFromPreferredAddress(params.PreferredAddress.ConnectionID, ¶ms.PreferredAddress.StatelessResetToken) + s.connIDManager.AddFromPreferredAddress(params.PreferredAddress.ConnectionID, params.PreferredAddress.StatelessResetToken) } // On the server side, the early session is ready as soon as we processed // the client's transport parameters. diff --git a/session_test.go b/session_test.go index 38b50f7c..d9f4dc79 100644 --- a/session_test.go +++ b/session_test.go @@ -654,7 +654,7 @@ var _ = Describe("Session", func() { streamManager.EXPECT().CloseWithError(gomock.Any()) sessionRunner.EXPECT().Remove(gomock.Any()).AnyTimes() cryptoSetup.EXPECT().Close() - sess.destroy(statelessResetErr{token: &token}) + sess.destroy(statelessResetErr{token: token}) }) })