Repository: pion/webrtc
Pull request #2077 (2022-01-07 15:35:02)

Fix RTPSender.Send crash
#### Description
If PeerConnection removes track while RTPSender is created and being negotiated,
RTPSender.Send would access nil pointer.
Check If track is nil.

I think it's related with issue #2065 

#### Reference issue
Fixes #...


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/2077?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#2077](https://codecov.io/gh/pion/webrtc/pull/2077?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (15435db) into [master](https://codecov.io/gh/pion/webrtc/commit/d2cc00dc40cbab9bf846563fc4716df87ed7b62b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (d2cc00d) will **decrease** coverage by `6.57%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pion/webrtc/pull/2077/graphs/tree.svg?width=650&height=150&src=pr&token=XOOuQo07I2&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)](https://codecov.io/gh/pion/webrtc/pull/2077?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

```diff
@@            Coverage Diff             @@
##           master    #2077      +/-   ##
==========================================
- Coverage   76.79%   70.22%   -6.58%     
==========================================
  Files          85       62      -23     
  Lines        8711     3231    -5480     
==========================================
- Hits         6690     2269    -4421     
+ Misses       1612      846     -766     
+ Partials      409      116     -293     
```

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

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/webrtc/pull/2077?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [constants.go](https://codecov.io/gh/pion/webrtc/pull/2077/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29uc3RhbnRzLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/2077/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-81.82%)` | :arrow_down: |
| [rtpcodec.go](https://codecov.io/gh/pion/webrtc/pull/2077/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cnRwY29kZWMuZ28=) | `13.33% <0.00%> (-80.00%)` | :arrow_down: |
| [track\_local.go](https://codecov.io/gh/pion/webrtc/pull/2077/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-dHJhY2tfbG9jYWwuZ28=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/2077/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-75.00%)` | :arrow_down: |
| [internal/mux/muxfunc.go](https://codecov.io/gh/pion/webrtc/pull/2077/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L211eGZ1bmMuZ28=) | `9.09% <0.00%> (-63.64%)` | :arrow_down: |
| [icemux.go](https://codecov.io/gh/pion/webrtc/pull/2077/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlbXV4Lmdv) | `0.00% <0.00%> (-54.55%)` | :arrow_down: |
| [atomicbool.go](https://codecov.io/gh/pion/webrtc/pull/2077/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-YXRvbWljYm9vbC5nbw==) | `57.14% <0.00%> (-42.86%)` | :arrow_down: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/2077/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c2Rwc2VtYW50aWNzLmdv) | `0.00% <0.00%> (-34.49%)` | :arrow_down: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/2077/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `24.13% <0.00%> (-34.49%)` | :arrow_down: |
| ... and [37 more](https://codecov.io/gh/pion/webrtc/pull/2077/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | |

------

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

Sean-Der: Oh this is fantastic! Thanks for adding more error handling, will merge after tests pass :)

Patch:
diff --git a/errors.go b/errors.go
index b40e5b39..a28389f5 100644
--- a/errors.go
+++ b/errors.go
@@ -205,6 +205,7 @@ var (
 	errRTPSenderTrackNil          = errors.New("Track must not be nil")
 	errRTPSenderDTLSTransportNil  = errors.New("DTLSTransport must not be nil")
 	errRTPSenderSendAlreadyCalled = errors.New("Send has already been called")
+	errRTPSenderTrackRemoved      = errors.New("Sender Track has been removed or replaced to nil")
 
 	errRTPTransceiverCannotChangeMid        = errors.New("errRTPSenderTrackNil")
 	errRTPTransceiverSetSendingInvalidState = errors.New("invalid state change in RTPTransceiver.setSending")
diff --git a/rtpsender.go b/rtpsender.go
index d0b60da5..80a4b72c 100644
--- a/rtpsender.go
+++ b/rtpsender.go
@@ -195,8 +195,11 @@ func (r *RTPSender) Send(parameters RTPSendParameters) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 
-	if r.hasSent() {
+	switch {
+	case r.hasSent():
 		return errRTPSenderSendAlreadyCalled
+	case r.track == nil:
+		return errRTPSenderTrackRemoved
 	}
 
 	writeStream := &interceptorToTrackLocalWriter{}


Test patch:
diff --git a/rtpsender_test.go b/rtpsender_test.go
index 4539b956..63e7e5fd 100644
--- a/rtpsender_test.go
+++ b/rtpsender_test.go
@@ -271,3 +271,59 @@ func Test_RTPSender_GetParameters_NilTrack(t *testing.T) {
 
 	assert.NoError(t, peerConnection.Close())
 }
+
+func Test_RTPSender_Send(t *testing.T) {
+	track, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeVP8}, "video", "pion")
+	assert.NoError(t, err)
+
+	peerConnection, err := NewPeerConnection(Configuration{})
+	assert.NoError(t, err)
+
+	rtpSender, err := peerConnection.AddTrack(track)
+	assert.NoError(t, err)
+
+	parameter := rtpSender.GetParameters()
+	err = rtpSender.Send(parameter)
+	<-rtpSender.sendCalled
+	assert.NoError(t, err)
+
+	assert.NoError(t, peerConnection.Close())
+}
+
+func Test_RTPSender_Send_Called_Once(t *testing.T) {
+	track, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeVP8}, "video", "pion")
+	assert.NoError(t, err)
+
+	peerConnection, err := NewPeerConnection(Configuration{})
+	assert.NoError(t, err)
+
+	rtpSender, err := peerConnection.AddTrack(track)
+	assert.NoError(t, err)
+
+	parameter := rtpSender.GetParameters()
+	err = rtpSender.Send(parameter)
+	<-rtpSender.sendCalled
+	assert.NoError(t, err)
+
+	err = rtpSender.Send(parameter)
+	assert.Equal(t, errRTPSenderSendAlreadyCalled, err)
+
+	assert.NoError(t, peerConnection.Close())
+}
+
+func Test_RTPSender_Send_Track_Removed(t *testing.T) {
+	track, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeVP8}, "video", "pion")
+	assert.NoError(t, err)
+
+	peerConnection, err := NewPeerConnection(Configuration{})
+	assert.NoError(t, err)
+
+	rtpSender, err := peerConnection.AddTrack(track)
+	assert.NoError(t, err)
+
+	parameter := rtpSender.GetParameters()
+	assert.NoError(t, peerConnection.RemoveTrack(rtpSender))
+	assert.Equal(t, errRTPSenderTrackRemoved, rtpSender.Send(parameter))
+
+	assert.NoError(t, peerConnection.Close())
+}