Repository: pion/webrtc
Pull request #1414 (2020-09-11 17:24:33)

Fix: CreateOffer race condition
Fixes a race condition that occurs when the local media (transceivers) are mutated during the offer generation process.

Resolves an issue where an offer is created with no `mid` causing the error below:
![image](https://user-images.githubusercontent.com/4740651/92954774-094ffa80-f432-11ea-9bb6-309be542960c.png)

The test is not great, since it depends on the performance of the executor. I was able to consistently reproduce locally with it though.

codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/1414?src=pr&el=h1) Report
> Merging [#1414](https://codecov.io/gh/pion/webrtc/pull/1414?src=pr&el=desc) into [master](https://codecov.io/gh/pion/webrtc/commit/fc11e7c66cff25daf73232502db785c0b15c814d?el=desc) will **decrease** coverage by `4.30%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pion/webrtc/pull/1414/graphs/tree.svg?width=650&height=150&src=pr&token=XOOuQo07I2)](https://codecov.io/gh/pion/webrtc/pull/1414?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1414      +/-   ##
==========================================
- Coverage   75.75%   71.45%   -4.31%     
==========================================
  Files          73       53      -20     
  Lines        5948     2305    -3643     
==========================================
- Hits         4506     1647    -2859     
+ Misses       1095      560     -535     
+ Partials      347       98     -249     
```

| Flag | Coverage Δ | |
|---|---|---|
| #go | `?` | |
| #wasm | `71.45% <ø> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/webrtc/pull/1414?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [icetcp.go](https://codecov.io/gh/pion/webrtc/pull/1414/diff?src=pr&el=tree#diff-aWNldGNwLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/1414/diff?src=pr&el=tree#diff-c2Rwc2VtYW50aWNzLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/1414/diff?src=pr&el=tree#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/1414/diff?src=pr&el=tree#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-71.80%)` | :arrow_down: |
| [gathering\_complete\_promise.go](https://codecov.io/gh/pion/webrtc/pull/1414/diff?src=pr&el=tree#diff-Z2F0aGVyaW5nX2NvbXBsZXRlX3Byb21pc2UuZ28=) | `57.14% <0.00%> (-42.86%)` | :arrow_down: |
| [dtlsrole.go](https://codecov.io/gh/pion/webrtc/pull/1414/diff?src=pr&el=tree#diff-ZHRsc3JvbGUuZ28=) | `70.58% <0.00%> (-29.42%)` | :arrow_down: |
| [icegathererstate.go](https://codecov.io/gh/pion/webrtc/pull/1414/diff?src=pr&el=tree#diff-aWNlZ2F0aGVyZXJzdGF0ZS5nbw==) | `75.00% <0.00%> (-25.00%)` | :arrow_down: |
| [networktype.go](https://codecov.io/gh/pion/webrtc/pull/1414/diff?src=pr&el=tree#diff-bmV0d29ya3R5cGUuZ28=) | `46.15% <0.00%> (-23.08%)` | :arrow_down: |
| [icecandidate.go](https://codecov.io/gh/pion/webrtc/pull/1414/diff?src=pr&el=tree#diff-aWNlY2FuZGlkYXRlLmdv) | `61.42% <0.00%> (-16.43%)` | :arrow_down: |
| [icecandidatetype.go](https://codecov.io/gh/pion/webrtc/pull/1414/diff?src=pr&el=tree#diff-aWNlY2FuZGlkYXRldHlwZS5nbw==) | `61.53% <0.00%> (-15.39%)` | :arrow_down: |
| ... and [22 more](https://codecov.io/gh/pion/webrtc/pull/1414/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/webrtc/pull/1414?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/webrtc/pull/1414?src=pr&el=footer). Last update [fc11e7c...b47c3da](https://codecov.io/gh/pion/webrtc/pull/1414?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/go.sum b/go.sum
index 861cc200..4379181e 100644
--- a/go.sum
+++ b/go.sum
@@ -172,6 +172,7 @@ github.com/shurcooL/users v0.0.0-20180125191416-49c67e49c537/go.mod h1:QJTqeLYED
 github.com/shurcooL/webdavfs v0.0.0-20170829043945-18c3829fa133/go.mod h1:hKmq5kWdCj2z2KEozexVbfEZIWiTjhE0+UjmZgPqehw=
 github.com/sourcegraph/annotate v0.0.0-20160123013949-f4cad6c6324d/go.mod h1:UdhH50NIW0fCiwBSr0co2m7BnFLdv4fQTgdqdJTHFeE=
 github.com/sourcegraph/syntaxhighlight v0.0.0-20170531221838-bd320f5d308e/go.mod h1:HuIsMU8RRBOtsCgI77wP899iHVBQpCmg4ErYMZB+2IA=
+github.com/stretchr/objx v0.1.0 h1:4G4v2dO3VZwixGIRoQ5Lfboy6nUhCyYzaqnIAPPhYs4=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
diff --git a/peerconnection.go b/peerconnection.go
index 6a0a05f8..24684955 100644
--- a/peerconnection.go
+++ b/peerconnection.go
@@ -348,7 +348,7 @@ func (pc *PeerConnection) checkNegotiationNeeded() bool {
 	for _, t := range pc.GetTransceivers() {
 		// https://www.w3.org/TR/webrtc/#dfn-update-the-negotiation-needed-flag
 		// Step 5.1
-		// if t.stoping && !t.stopped {
+		// if t.stopping && !t.stopped {
 		// 	return true
 		// }
 		m := getByMid(t.Mid(), localDesc)
@@ -541,7 +541,23 @@ func (pc *PeerConnection) getStatsID() string {
 	return pc.statsID
 }
 
+func (pc *PeerConnection) hasLocalDescriptionChanged(desc *SessionDescription) bool {
+	for _, t := range pc.GetTransceivers() {
+		m := getByMid(t.Mid(), desc)
+		if m == nil {
+			return true
+		}
+
+		if getPeerDirection(m) != t.Direction() {
+			return true
+		}
+	}
+
+	return false
+}
+
 // CreateOffer starts the PeerConnection and generates the localDescription
+// https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-createoffer
 func (pc *PeerConnection) CreateOffer(options *OfferOptions) (SessionDescription, error) {
 	useIdentity := pc.idpLoginURL != nil
 	switch {
@@ -557,67 +573,90 @@ func (pc *PeerConnection) CreateOffer(options *OfferOptions) (SessionDescription
 		}
 	}
 
-	isPlanB := pc.configuration.SDPSemantics == SDPSemanticsPlanB
-	if pc.currentRemoteDescription != nil {
-		isPlanB = descriptionIsPlanB(pc.RemoteDescription())
-	}
+	var (
+		d     *sdp.SessionDescription
+		offer SessionDescription
+		err   error
+	)
 
-	// include unmatched local transceivers
-	if !isPlanB {
-		// update the greater mid if the remote description provides a greater one
+	// This may be necessary to recompute if, for example, createOffer was called when only an
+	// audio RTCRtpTransceiver was added to connection, but while performing the in-parallel
+	// steps to create an offer, a video RTCRtpTransceiver was added, requiring additional
+	// inspection of video system resources.
+	for {
+		// We cache current transceivers to ensure they aren't
+		// mutated during offer generation. We later check if they have
+		// been mutated and recompute the offer if necessary.
+		currentTransceivers := pc.GetTransceivers()
+
+		// in-parallel steps to create an offer
+		// https://w3c.github.io/webrtc-pc/#dfn-in-parallel-steps-to-create-an-offer
+		isPlanB := pc.configuration.SDPSemantics == SDPSemanticsPlanB
 		if pc.currentRemoteDescription != nil {
-			for _, media := range pc.currentRemoteDescription.parsed.MediaDescriptions {
-				mid := getMidValue(media)
-				if mid == "" {
-					continue
+			isPlanB = descriptionIsPlanB(pc.RemoteDescription())
+		}
+
+		// include unmatched local transceivers
+		if !isPlanB {
+			// update the greater mid if the remote description provides a greater one
+			if pc.currentRemoteDescription != nil {
+				var numericMid int
+				for _, media := range pc.currentRemoteDescription.parsed.MediaDescriptions {
+					mid := getMidValue(media)
+					if mid == "" {
+						continue
+					}
+					numericMid, err = strconv.Atoi(mid)
+					if err != nil {
+						continue
+					}
+					if numericMid > pc.greaterMid {
+						pc.greaterMid = numericMid
+					}
 				}
-				numericMid, err := strconv.Atoi(mid)
-				if err != nil {
+			}
+			for _, t := range currentTransceivers {
+				if t.Mid() != "" {
 					continue
 				}
-				if numericMid > pc.greaterMid {
-					pc.greaterMid = numericMid
+				pc.greaterMid++
+				err = t.setMid(strconv.Itoa(pc.greaterMid))
+				if err != nil {
+					return SessionDescription{}, err
 				}
 			}
 		}
-		for _, t := range pc.GetTransceivers() {
-			if t.Mid() != "" {
-				continue
-			}
-			pc.greaterMid++
-			err := t.setMid(strconv.Itoa(pc.greaterMid))
-			if err != nil {
-				return SessionDescription{}, err
-			}
+
+		if pc.currentRemoteDescription == nil {
+			d, err = pc.generateUnmatchedSDP(currentTransceivers, useIdentity)
+		} else {
+			d, err = pc.generateMatchedSDP(currentTransceivers, useIdentity, true /*includeUnmatched */, connectionRoleFromDtlsRole(defaultDtlsRoleOffer))
 		}
-	}
 
-	var (
-		d   *sdp.SessionDescription
-		err error
-	)
+		if err != nil {
+			return SessionDescription{}, err
+		}
 
-	if pc.currentRemoteDescription == nil {
-		d, err = pc.generateUnmatchedSDP(useIdentity)
-	} else {
-		d, err = pc.generateMatchedSDP(useIdentity, true /*includeUnmatched */, connectionRoleFromDtlsRole(defaultDtlsRoleOffer))
-	}
-	if err != nil {
-		return SessionDescription{}, err
-	}
+		sdpBytes, err := d.Marshal()
+		if err != nil {
+			return SessionDescription{}, err
+		}
 
-	sdpBytes, err := d.Marshal()
-	if err != nil {
-		return SessionDescription{}, err
-	}
+		offer = SessionDescription{
+			Type:   SDPTypeOffer,
+			SDP:    string(sdpBytes),
+			parsed: d,
+		}
 
-	desc := SessionDescription{
-		Type:   SDPTypeOffer,
-		SDP:    string(sdpBytes),
-		parsed: d,
+		// Verify local media hasn't changed during offer
+		// generation. Recompute if necessary
+		if isPlanB || !pc.hasLocalDescriptionChanged(&offer) {
+			break
+		}
 	}
-	pc.lastOffer = desc.SDP
-	return desc, nil
+
+	pc.lastOffer = offer.SDP
+	return offer, nil
 }
 
 func (pc *PeerConnection) createICEGatherer() (*ICEGatherer, error) {
@@ -725,7 +764,8 @@ func (pc *PeerConnection) CreateAnswer(options *AnswerOptions) (SessionDescripti
 		connectionRole = connectionRoleFromDtlsRole(defaultDtlsRoleAnswer)
 	}
 
-	d, err := pc.generateMatchedSDP(useIdentity, false /*includeUnmatched */, connectionRole)
+	currentTransceivers := pc.GetTransceivers()
+	d, err := pc.generateMatchedSDP(currentTransceivers, useIdentity, false /*includeUnmatched */, connectionRole)
 	if err != nil {
 		return SessionDescription{}, err
 	}
@@ -1462,8 +1502,6 @@ func (pc *PeerConnection) AddTrack(track *Track) (*RTPSender, error) {
 		return nil, err
 	}
 
-	pc.onNegotiationNeeded()
-
 	return transceiver.Sender(), nil
 }
 
@@ -1999,7 +2037,7 @@ func (pc *PeerConnection) GetRegisteredRTPCodecs(kind RTPCodecType) []*RTPCodec
 
 // generateUnmatchedSDP generates an SDP that doesn't take remote state into account
 // This is used for the initial call for CreateOffer
-func (pc *PeerConnection) generateUnmatchedSDP(useIdentity bool) (*sdp.SessionDescription, error) {
+func (pc *PeerConnection) generateUnmatchedSDP(transceivers []*RTPTransceiver, useIdentity bool) (*sdp.SessionDescription, error) {
 	d, err := sdp.NewJSEPSessionDescription(useIdentity)
 	if err != nil {
 		return nil, err
@@ -2022,7 +2060,7 @@ func (pc *PeerConnection) generateUnmatchedSDP(useIdentity bool) (*sdp.SessionDe
 		video := make([]*RTPTransceiver, 0)
 		audio := make([]*RTPTransceiver, 0)
 
-		for _, t := range pc.GetTransceivers() {
+		for _, t := range transceivers {
 			if t.kind == RTPCodecTypeVideo {
 				video = append(video, t)
 			} else if t.kind == RTPCodecTypeAudio {
@@ -2044,7 +2082,7 @@ func (pc *PeerConnection) generateUnmatchedSDP(useIdentity bool) (*sdp.SessionDe
 			mediaSections = append(mediaSections, mediaSection{id: "data", data: true})
 		}
 	} else {
-		for _, t := range pc.GetTransceivers() {
+		for _, t := range transceivers {
 			if t.Sender() != nil {
 				t.Sender().setNegotiated()
 			}
@@ -2067,7 +2105,7 @@ func (pc *PeerConnection) generateUnmatchedSDP(useIdentity bool) (*sdp.SessionDe
 // generateMatchedSDP generates a SDP and takes the remote state into account
 // this is used everytime we have a RemoteDescription
 // nolint: gocyclo
-func (pc *PeerConnection) generateMatchedSDP(useIdentity bool, includeUnmatched bool, connectionRole sdp.ConnectionRole) (*sdp.SessionDescription, error) {
+func (pc *PeerConnection) generateMatchedSDP(transceivers []*RTPTransceiver, useIdentity bool, includeUnmatched bool, connectionRole sdp.ConnectionRole) (*sdp.SessionDescription, error) {
 	d, err := sdp.NewJSEPSessionDescription(useIdentity)
 	if err != nil {
 		return nil, err
@@ -2084,7 +2122,7 @@ func (pc *PeerConnection) generateMatchedSDP(useIdentity bool, includeUnmatched
 	}
 
 	var t *RTPTransceiver
-	localTransceivers := append([]*RTPTransceiver{}, pc.GetTransceivers()...)
+	localTransceivers := append([]*RTPTransceiver{}, transceivers...)
 	detectedPlanB := descriptionIsPlanB(pc.RemoteDescription())
 	mediaSections := []mediaSection{}
 	alreadyHaveApplicationMediaSection := false


Test patch:
diff --git a/peerconnection_renegotiation_test.go b/peerconnection_renegotiation_test.go
index ae8d62d7..86527f22 100644
--- a/peerconnection_renegotiation_test.go
+++ b/peerconnection_renegotiation_test.go
@@ -899,3 +899,39 @@ func TestNegotiationNeededRemoveTrack(t *testing.T) {
 	assert.NoError(t, pcOffer.Close())
 	assert.NoError(t, pcAnswer.Close())
 }
+
+func TestNegotiationNeededStressOneSided(t *testing.T) {
+	api := NewAPI()
+	lim := test.TimeOut(time.Second * 30)
+	defer lim.Stop()
+
+	report := test.CheckRoutines(t)
+	defer report()
+
+	api.mediaEngine.RegisterDefaultCodecs()
+	pcA, pcB, err := api.newPair(Configuration{})
+	assert.NoError(t, err)
+	defer pcA.Close()
+	defer pcB.Close()
+
+	pcA.OnNegotiationNeeded(func() {
+		assert.NoError(t, signalPair(pcA, pcB))
+	})
+
+	for i := 0; i < 500; i++ {
+		time.Sleep(10 * time.Millisecond)
+
+		track, err := pcA.NewTrack(DefaultPayloadTypeVP8, rand.Uint32(), "video", "pion")
+		assert.NoError(t, err)
+
+		_, err = pcA.AddTrack(track)
+		assert.NoError(t, err)
+
+		err = track.WriteSample(media.Sample{Data: []byte{0x00}, Samples: 1})
+		assert.NoError(t, err)
+	}
+
+	// Wait for async to complete otherwise dangling renegotation
+	// will occur on closed connection and throw InvalidStateError: connection closed
+	time.Sleep(time.Duration(2) * time.Second)
+}