Sync with libwebrtc M144

Author: fippo

agent.go

@@ -1866,6 +1866,9 @@ func (a *Agent) handleInboundRequest(
 	remoteTieBreaker := &AttrControl{}
 	if err := remoteTieBreaker.GetFrom(msg); err == nil && remoteTieBreaker.Role == a.role() {
 		a.handleRoleConflict(msg, local, remoteCandidate, remoteTieBreaker)
+		// The selector does not get to see this message, report any piggybacked
+		// DTLS to avoid the peer having to retransmit it.
+		a.reportPiggybackingFromMessage(msg, remoteCandidate)
 
 		return nil, false
 	}

piggyback.go

@@ -6,6 +6,7 @@
 package ice
 
 import (
+	"errors"
 	"hash/crc32"
 	"net"
 	"slices"
@@ -19,6 +20,15 @@ type packetWithCrc struct {
 	crc  uint32
 }
 
+// dtlsRecordHeaderLen is the length of the DTLS record header.
+const dtlsRecordHeaderLen = 13
+
+// isDtlsPacket determines whether the payload is a DTLS record. Only those are
+// embedded in or extracted from STUN messages.
+func isDtlsPacket(payload []byte) bool {
+	return len(payload) >= dtlsRecordHeaderLen && payload[0] > 19 && payload[0] < 64
+}
+
 type piggybackingState int
 
 const (
@@ -39,6 +49,8 @@ type piggybackingController struct {
 	dtlsCallback func(packet []byte, rAddr net.Addr)
 	newFlight    bool
 	connected    bool
+	// DTLS 1.2 only. For DTLS 1.3 the role this matters for is reversed.
+	isDtlsClient bool
 }
 
 // init sets the controller to its initial off state. SetDtlsCallback flips it
@@ -77,6 +89,25 @@ func (a *Agent) SetDtlsCallback(cb func(packet []byte, rAddr net.Addr)) {
 	}
 }
 
+// SetDtlsRole informs the controller whether we are the DTLS client. It needs
+// to be called before the local DTLS handshake completes since it determines
+// whether the last flight has to be kept around for retransmission.
+func (a *Agent) SetDtlsRole(isClient bool) {
+	a.piggyback.mu.Lock()
+	defer a.piggyback.mu.Unlock()
+	a.piggyback.isDtlsClient = isClient
+}
+
+// SetDtlsFailed disables piggybacking after the DTLS handshake failed.
+func (a *Agent) SetDtlsFailed() {
+	a.piggyback.mu.Lock()
+	defer a.piggyback.mu.Unlock()
+	if a.piggyback.state != PiggybackingStateComplete && a.piggyback.state != PiggybackingStateOff {
+		a.log.Info("DTLS failed during negotiation, disabling piggybacking")
+	}
+	a.piggyback.state = PiggybackingStateOff
+}
+
 // Piggyback stores a packet to be picked in a round-robin fashion.
 // Returns `true` if packet is to be consumed.
 // A nil packet signals that the local DTLS handshake completed.
@@ -88,6 +119,10 @@ func (a *Agent) Piggyback(packet []byte, end bool) bool {
 	}
 
 	if packet != nil {
+		// Anything that is not DTLS gets sent as-is.
+		if !isDtlsPacket(packet) {
+			return false
+		}
 		// If we receive a packet after the end of a flight we need
 		// to clear the outgoing list.
 		if a.piggyback.newFlight {
@@ -97,7 +132,17 @@ func (a *Agent) Piggyback(packet []byte, end bool) bool {
 		a.piggyback.newFlight = end
 		crc := crc32.ChecksumIEEE(packet)
 		a.piggyback.packets = append(a.piggyback.packets, packetWithCrc{packet, crc})
-	} else if a.piggyback.state != PiggybackingStateOff {
+	} else if a.piggyback.state != PiggybackingStateOff && a.piggyback.state != PiggybackingStateComplete {
+		// As DTLS 1.2 client we have nothing more to send at this point but
+		// will continue to send ACK attributes until receiving the last flight
+		// from the server. As DTLS 1.2 server we need to keep the last flight
+		// around until we receive the post-handshake acknowledgment.
+		// For DTLS 1.3 this is reversed since the handshake is one round trip
+		// shorter.
+		if a.piggyback.isDtlsClient {
+			a.piggyback.packets = []packetWithCrc{}
+			a.piggyback.packetsIndex = 0
+		}
 		a.piggyback.state = PiggybackingStatePending
 	}
 	// If we are connected we could send DTLS plain.
@@ -113,17 +158,18 @@ func (a *Agent) GetPiggybackDataAndAcks() ([]byte, []uint32) {
 		return nil, nil
 	}
 	if len(a.piggyback.packets) == 0 {
-		return nil, a.piggyback.acks
+		return nil, slices.Clone(a.piggyback.acks)
 	}
 
 	packet := a.piggyback.packets[a.piggyback.packetsIndex]
 	a.piggyback.packetsIndex = (a.piggyback.packetsIndex + 1) % len(a.piggyback.packets)
 
-	// Return a copy to prevent external modification of the internal buffer
+	// Return copies to prevent external modification of the internal buffers
+	// which are written to while the message is built.
 	result := make([]byte, len(packet.data))
 	copy(result, packet.data)
 
-	return result, a.piggyback.acks
+	return result, slices.Clone(a.piggyback.acks)
 }
 
 func (a *Agent) ReportPiggybacking(packet []byte, acks []uint32, rAddr net.Addr) { //nolint:cyclop
@@ -143,8 +189,10 @@ func (a *Agent) ReportPiggybacking(packet []byte, acks []uint32, rAddr net.Addr)
 
 		return
 	}
-	if packet == nil && acks == nil && a.piggyback.acks != nil {
-		a.log.Infof("Done with the SPED handshake", a.piggyback.state)
+	// In the pending state the peer may have stopped sending acks when it
+	// moved to the complete state. Move to the same state.
+	if packet == nil && acks == nil && a.piggyback.state == PiggybackingStatePending {
+		a.log.Info("Done with the SPED handshake")
 		a.piggyback.acks = nil
 		a.piggyback.state = PiggybackingStateComplete
 		a.piggyback.mu.Unlock()
@@ -170,9 +218,29 @@ func (a *Agent) ReportPiggybacking(packet []byte, acks []uint32, rAddr net.Addr)
 			a.piggyback.packetsIndex = 0
 		}
 	}
-	if len(packet) == 0 {
+	// The response to the final flight of the handshake will not contain the
+	// DTLS data but will contain an ack. Must not happen on the initial server
+	// to client packet which has no DTLS data yet.
+	if packet == nil && acks != nil && a.piggyback.state == PiggybackingStatePending {
+		a.log.Info("Done with the SPED handshake")
+		a.piggyback.acks = nil
+		a.piggyback.state = PiggybackingStateComplete
+		a.piggyback.mu.Unlock()
+
+		return
+	}
+	// Keep sending the (possibly empty) ack attribute as long as we are active,
+	// its absence signals that the handshake is done.
+	if a.piggyback.acks == nil {
 		a.piggyback.acks = []uint32{}
 	}
+	// Drop non-DTLS data.
+	if len(packet) > 0 && !isDtlsPacket(packet) {
+		a.log.Warn("Dropping non-DTLS data")
+		a.piggyback.mu.Unlock()
+
+		return
+	}
 
 	var dtlsCallback func(packet []byte, rAddr net.Addr)
 	// Handle the incoming packet. Calculate and store the crc32 of the packet
@@ -214,10 +282,21 @@ func (a *Agent) appendPiggybackAttributes(attrs []stun.Setter) []stun.Setter {
 // reportPiggybackingFromMessage extracts the DTLS-in-STUN payload and ACK list
 // from a STUN message and forwards them to the controller.
 func (a *Agent) reportPiggybackingFromMessage(message *stun.Message, remote Candidate) {
+	// A malformed attribute must not be treated like an absent one which
+	// signals that the peer does not support piggybacking or is done with the
+	// handshake. Drop the message instead.
 	var dtls DtlsInStunAttribute
-	_ = dtls.GetFrom(message)
+	if err := dtls.GetFrom(message); err != nil && !errors.Is(err, stun.ErrAttributeNotFound) {
+		a.log.Warnf("Discarding malformed DTLS-in-STUN attribute: %v", err)
+
+		return
+	}
 	var ack DtlsInStunAckAttribute
-	_ = ack.GetFrom(message)
+	if err := ack.GetFrom(message); err != nil && !errors.Is(err, stun.ErrAttributeNotFound) {
+		a.log.Warnf("Discarding malformed DTLS-in-STUN ack attribute: %v", err)
+
+		return
+	}
 	a.ReportPiggybacking(dtls, ack, remote.addr())
 }
 

piggyback_test.go

@@ -14,6 +14,12 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// fakeDtlsPacket prefixes the payload with a DTLS 1.2 handshake record header
+// so it is recognized as a DTLS packet.
+func fakeDtlsPacket(payload string) []byte {
+	return append([]byte{22, 0xfe, 0xfd, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, []byte(payload)...)
+}
+
 func TestSped(t *testing.T) {
 	defer test.CheckRoutines(t)()
 
@@ -25,12 +31,12 @@ func TestSped(t *testing.T) {
 		require.NoError(t, err)
 		require.NoError(t, aAgent.OnConnectionStateChange(aNotifier))
 
-		var toA string
-		fromA := "Hello from A"
+		var toA []byte
+		fromA := fakeDtlsPacket("Hello from A")
 		aAgent.SetDtlsCallback(func(packet []byte, rAddr net.Addr) {
-			toA = string(packet)
+			toA = packet
 		})
-		require.True(t, aAgent.Piggyback([]byte(fromA), true))
+		require.True(t, aAgent.Piggyback(fromA, true))
 
 		bNotifier, bConnected := onConnected()
 		bAgent, err := NewAgent(&AgentConfig{
@@ -39,12 +45,12 @@ func TestSped(t *testing.T) {
 		require.NoError(t, err)
 		require.NoError(t, bAgent.OnConnectionStateChange(bNotifier))
 
-		var toB string
-		fromB := "Hello from B"
+		var toB []byte
+		fromB := fakeDtlsPacket("Hello from B")
 		bAgent.SetDtlsCallback(func(packet []byte, rAddr net.Addr) {
-			toB = string(packet)
+			toB = packet
 		})
-		require.True(t, bAgent.Piggyback([]byte(fromB), true))
+		require.True(t, bAgent.Piggyback(fromB, true))
 
 		gatherAndExchangeCandidates(t, aAgent, bAgent)
 		go func() {