webrtc icon indicating copy to clipboard operation
webrtc copied to clipboard
verified publisher icon pion

fix: rtcp interceptor nil panic

Open LeeTeng2001 opened this issue 1 year ago • 10 comments

Description

Add a nil rtpinterceptor check before invoking read method

Reference issue

Fixes #2929

LeeTeng2001 avatar Oct 14 '24 02:10 LeeTeng2001

Thank you @LeeTeng2001!

How can I reproduce this? I would like to add a test before merging!

Sean-Der avatar Oct 14 '24 02:10 Sean-Der

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.24%. Comparing base (1c45355) to head (7244222). Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2930      +/-   ##
==========================================
+ Coverage   78.22%   78.24%   +0.02%     
==========================================
  Files          91       91              
  Lines       11125    11128       +3     
==========================================
+ Hits         8702     8707       +5     
+ Misses       1935     1933       -2     
  Partials      488      488
Flag Coverage Δ
go 80.12% <100.00%> (+0.02%) :arrow_up:
wasm 63.24% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 14 '24 02:10 codecov[bot]

hi, can you give me some idea on how to write a unit test that cover this part of code? I would love to write one without spinning up a full webrtc connection. Actually I don't really know which part of the negotiation process caused my code to panic.

LeeTeng2001 avatar Oct 14 '24 09:10 LeeTeng2001

I would like to write a very minimal code to trigger the track receiver to peek without hooking up full webrtc framework. This is what I got so far, but it ran into deadlock, can you give me some idea on how I should achive this?

func Test_RTPReceiver_NilPanic(t *testing.T) {
	api := NewAPI()
	// Create the ICE gatherer
	gatherer, err := api.NewICEGatherer(ICEGatherOptions{})
	assert.NoError(t, err)
	ice := api.NewICETransport(gatherer)
	dtls, err := api.NewDTLSTransport(ice, nil)
	assert.NoError(t, err)
	rtp, err := api.NewRTPReceiver(RTPCodecTypeVideo, dtls)
	assert.NoError(t, err)
	tr := newTrackRemote(
		RTPCodecTypeVideo,
		0,
		0,
		"rid",
		rtp,
	)
	var wg sync.WaitGroup
	wg.Add(1)
	go func() {
		defer wg.Done()
		tr.peek([]byte{})
	}()

	wg.Wait()
}

LeeTeng2001 avatar Oct 14 '24 09:10 LeeTeng2001

@LeeTeng2001 Do you have an example/can you explain how to reproduce with the full WebRTC? I can make it more minimal after that!

sorry don’t fully understand yet

Sean-Der avatar Oct 14 '24 13:10 Sean-Der

@Sean-Der I can't provide the full example as it's confidential, can I provide error trace point instead? This panic happens after local descriptor has gather completed. When I run my code in debug mode, I noticed that the track codec is empty, is this expected behaviour? image

I tested and this issue is still present on v4 version of webrtc, I thought this issue might be fixed with #2610, are they related?

LeeTeng2001 avatar Oct 16 '24 02:10 LeeTeng2001

The track at the panic point has a bunce of nil interceptor and read stream image

LeeTeng2001 avatar Oct 16 '24 02:10 LeeTeng2001

Relevant debug output before panic

[handshake] use cipher suite: TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
[handshake:client] Flight 1 -> Flight 5
[handshake:client] Flight 5: Preparing
[handshake:client] -> changeCipherSpec (epoch: 1)
[handshake:client] Flight 5: Sending
[handshake:client] -> TypeCertificate (epoch: 0, seq: 1)
[handshake:client] -> ClientKeyExchange (epoch: 0, seq: 2)
[handshake:client] -> CertificateVerify (epoch: 0, seq: 3)
[handshake:client] -> Finished (epoch: 1, seq: 4)
[handshake:client] Flight 5: Waiting
client: <- ChangeCipherSpec (epoch: 1)
[handshake:client] Flight 5 -> Flight 5
[handshake:client] Flight 5: Finished
Handshake Completed
peer connection state changed: connected
[0xc0002643c0] updated cwnd=4380 ssthresh=0 inflight=0 (INI)
[0xc0002643c0] sending INIT
[0xc0002643c0] state change: 'Closed' => 'CookieWait'
panic: runtime error: invalid memory address or nil pointer dereference

LeeTeng2001 avatar Oct 16 '24 03:10 LeeTeng2001

Can you provide the Offer/Answer that causes the issue?

I just want to understand/fix the root cause

Sean-Der avatar Oct 16 '24 03:10 Sean-Der

Sure: Offer:

v=0
o=- 9170416573049417688 2 IN IP4 127.0.0.1
s=-
t=0 0
a=group:BUNDLE 0 1 2
a=extmap-allow-mixed
a=msid-semantic: WMS cg
m=audio 9 UDP/TLS/RTP/SAVPF 111 110
c=IN IP4 0.0.0.0
a=rtcp:9 IN IP4 0.0.0.0
a=ice-ufrag:ATAe
a=ice-pwd:OtDV95gBWWQtNkd1gRjSKf80
a=ice-options:trickle
a=fingerprint:sha-256 AF:35:DA:66:C9:CF:14:84:10:0D:D3:FC:E3:70:54:4C:99:C4:F3:D4:8F:65:F2:84:82:61:2A:E3:ED:24:8C:F3
a=setup:actpass
a=mid:0
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=extmap:2 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a\=extmap:3 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a\=extmap:4 urn:ietf:params:rtp-hdrext:sdes:mid
a=extmap:5 urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id
a=extmap:6 urn:ietf:params:rtp-hdrext:sdes:repaired-rtp-stream-id
a=sendonly
a=msid:cg audio_track
a=rtcp-mux
a=rtpmap:111 opus/48000/2
a=rtcp-fb:111 transport-cc
a=rtcp-fb:111 nack
a=fmtp:111 maxaveragebitrate=192000;minptime=10;stereo=1;useinbandfec=1
a=rtpmap:110 telephone-event/48000
a=ptime:10
a=ssrc:326266142 cname:AMUuBeJ7qSxx2UZM
a=ssrc:326266142 msid:cg audio_track
a=ssrc:326266142 mslabel:cg
a=ssrc:326266142 label:audio_track
m=video 9 UDP/TLS/RTP/SAVPF 96 97 98 99 100 101 102 103 104 105 106 107 108 109 127 126 112 113 116 117 118 35 36
c=IN IP4 0.0.0.0
a=rtcp:9 IN IP4 0.0.0.0
a=ice-ufrag:ATAe
a=ice-pwd:OtDV95gBWWQtNkd1gRjSKf80
a=ice-options:trickle
a=fingerprint:sha-256 AF:35:DA:66:C9:CF:14:84:10:0D:D3:FC:E3:70:54:4C:99:C4:F3:D4:8F:65:F2:84:82:61:2A:E3:ED:24:8C:F3
a=setup:actpass
a=mid:1
a=extmap:14 urn:ietf:params:rtp-hdrext:toffset
a=extmap:2 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a\=extmap:13 urn:3gpp:video-orientation
a=extmap:3 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a\=extmap:12 http://www.webrtc.org/experiments/rtp-hdrext/playout-delay
a\=extmap:11 http://www.webrtc.org/experiments/rtp-hdrext/video-content-type
a\=extmap:7 http://www.webrtc.org/experiments/rtp-hdrext/video-timing
a\=extmap:8 http://www.webrtc.org/experiments/rtp-hdrext/color-space
a\=extmap:4 urn:ietf:params:rtp-hdrext:sdes:mid
a=extmap:5 urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id
a=extmap:6 urn:ietf:params:rtp-hdrext:sdes:repaired-rtp-stream-id
a=extmap:10 urn:mihoyo:frame-header
a=extmap:9 urn:mihoyo:trace-time
a=sendonly
a=msid:cg video_track
a=rtcp-mux
a=rtcp-rsize
a=rtpmap:96 H265/90000
a=rtcp-fb:96 goog-remb
a=rtcp-fb:96 transport-cc
a=rtcp-fb:96 ccm fir
a=rtcp-fb:96 nack
a=rtcp-fb:96 nack pli
a=fmtp:96 level-id=120;profile-id=1;profile-space=0;tier-flag=0
a=rtpmap:97 rtx/90000
a=fmtp:97 apt=96
a=rtpmap:98 H264/90000
a=rtcp-fb:98 goog-remb
a=rtcp-fb:98 transport-cc
a=rtcp-fb:98 ccm fir
a=rtcp-fb:98 nack
a=rtcp-fb:98 nack pli
a=fmtp:98 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=640034
a=rtpmap:99 rtx/90000
a=fmtp:99 apt=98
a=rtpmap:100 H264/90000
a=rtcp-fb:100 goog-remb
a=rtcp-fb:100 transport-cc
a=rtcp-fb:100 ccm fir
a=rtcp-fb:100 nack
a=rtcp-fb:100 nack pli
a=fmtp:100 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=640034
a=rtpmap:101 rtx/90000
a=fmtp:101 apt=100
a=rtpmap:102 H264/90000
a=rtcp-fb:102 goog-remb
a=rtcp-fb:102 transport-cc
a=rtcp-fb:102 ccm fir
a=rtcp-fb:102 nack
a=rtcp-fb:102 nack pli
a=fmtp:102 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=4d0029
a=rtpmap:103 rtx/90000
a=fmtp:103 apt=102
a=rtpmap:104 H264/90000
a=rtcp-fb:104 goog-remb
a=rtcp-fb:104 transport-cc
a=rtcp-fb:104 ccm fir
a=rtcp-fb:104 nack
a=rtcp-fb:104 nack pli
a=fmtp:104 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=4d0029
a=rtpmap:105 rtx/90000
a=fmtp:105 apt=104
a=rtpmap:106 H264/90000
a=rtcp-fb:106 goog-remb
a=rtcp-fb:106 transport-cc
a=rtcp-fb:106 ccm fir
a=rtcp-fb:106 nack
a=rtcp-fb:106 nack pli
a=fmtp:106 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42001f
a=rtpmap:107 rtx/90000
a=fmtp:107 apt=106
a=rtpmap:108 H264/90000
a=rtcp-fb:108 goog-remb
a=rtcp-fb:108 transport-cc
a=rtcp-fb:108 ccm fir
a=rtcp-fb:108 nack
a=rtcp-fb:108 nack pli
a=fmtp:108 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=42001f
a=rtpmap:109 rtx/90000
a=fmtp:109 apt=108
a=rtpmap:127 H264/90000
a=rtcp-fb:127 goog-remb
a=rtcp-fb:127 transport-cc
a=rtcp-fb:127 ccm fir
a=rtcp-fb:127 nack
a=rtcp-fb:127 nack pli
a=fmtp:127 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f
a=rtpmap:126 rtx/90000
a=fmtp:126 apt=127
a=rtpmap:112 H264/90000
a=rtcp-fb:112 goog-remb
a=rtcp-fb:112 transport-cc
a=rtcp-fb:112 ccm fir
a=rtcp-fb:112 nack
a=rtcp-fb:112 nack pli
a=fmtp:112 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=42e01f
a=rtpmap:113 rtx/90000
a=fmtp:113 apt=112
a=rtpmap:116 red/90000
a=rtpmap:117 rtx/90000
a=fmtp:117 apt=116
a=rtpmap:118 ulpfec/90000
a=rtpmap:35 flexfec-03/90000
a=rtcp-fb:35 goog-remb
a=rtcp-fb:35 transport-cc
a=fmtp:35 repair-window=10000000
a=rtpmap:36 rsfec/90000
a=rtcp-fb:36 goog-remb
a=rtcp-fb:36 transport-cc
a=fmtp:36 repair-window=10000000
a=ssrc-group:FID 2919416784 2402605702
a=ssrc-group:FEC-FR 2919416784 1409269607
a=ssrc:2919416784 cname:AMUuBeJ7qSxx2UZM
a=ssrc:2919416784 msid:cg video_track
a=ssrc:2919416784 mslabel:cg
a=ssrc:2919416784 label:video_track
a=ssrc:2402605702 cname:AMUuBeJ7qSxx2UZM
a=ssrc:2402605702 msid:cg video_track
a=ssrc:2402605702 mslabel:cg
a=ssrc:2402605702 label:video_track
a=ssrc:1409269607 cname:AMUuBeJ7qSxx2UZM
a=ssrc:1409269607 msid:cg video_track
a=ssrc:1409269607 mslabel:cg
a=ssrc:1409269607 label:video_track
m=application 9 UDP/DTLS/SCTP webrtc-datachannel
c=IN IP4 0.0.0.0
a=ice-ufrag:ATAe
a=ice-pwd:OtDV95gBWWQtNkd1gRjSKf80
a=ice-options:trickle
a=fingerprint:sha-256 AF:35:DA:66:C9:CF:14:84:10:0D:D3:FC:E3:70:54:4C:99:C4:F3:D4:8F:65:F2:84:82:61:2A:E3:ED:24:8C:F3
a=setup:actpass
a=mid:2
a=sctp-port:5000
a=max-message-size:262144

Answer:

v=0
o=- 6394559322765338330 1729148681 IN IP4 0.0.0.0
s=-
t=0 0
a=msid-semantic:WMS*
a=fingerprint:sha-256 EB:36:64:B0:55:8E:85:32:D7:61:43:C2:54:F7:8D:F2:72:66:A7:8D:D4:2F:C5:BF:F0:4F:42:B2:60:51:8C:A3
a=extmap-allow-mixed
a=group:BUNDLE 0 1 2
m=audio 9 UDP/TLS/RTP/SAVPF 111
c=IN IP4 0.0.0.0
a=setup:active
a=mid:0
a=ice-ufrag:ZiFDSBbcTBZWsXza
a=ice-pwd:zkRSAmJQoNZZWrSNpDjImDnUZnovVsxH
a=rtcp-mux
a=rtcp-rsize
a=rtpmap:111 opus/48000/2
a=fmtp:111 maxaveragebitrate=192000;minptime=10;stereo=1;useinbandfec=1
a=rtcp-fb:111 transport-cc
a=rtcp-fb:111 nack
a=extmap:3 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a\=recvonly
m\=video 9 UDP/TLS/RTP/SAVPF 98 99 102 103 104 105 106 107 108 109 127 126 112 113
c=IN IP4 0.0.0.0
a=setup:active
a=mid:1
a=ice-ufrag:ZiFDSBbcTBZWsXza
a=ice-pwd:zkRSAmJQoNZZWrSNpDjImDnUZnovVsxH
a=rtcp-mux
a=rtcp-rsize
a=rtpmap:98 H264/90000
a=fmtp:98 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=640034
a=rtcp-fb:98 goog-remb
a=rtcp-fb:98 transport-cc
a=rtcp-fb:98 ccm fir
a=rtcp-fb:98 nack
a=rtcp-fb:98 nack pli
a=rtpmap:99 rtx/90000
a=fmtp:99 apt=98
a=rtpmap:102 H264/90000
a=fmtp:102 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=4d0029
a=rtcp-fb:102 goog-remb
a=rtcp-fb:102 transport-cc
a=rtcp-fb:102 ccm fir
a=rtcp-fb:102 nack
a=rtcp-fb:102 nack pli
a=rtpmap:103 rtx/90000
a=fmtp:103 apt=102
a=rtpmap:104 H264/90000
a=fmtp:104 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=4d0029
a=rtcp-fb:104 goog-remb
a=rtcp-fb:104 transport-cc
a=rtcp-fb:104 ccm fir
a=rtcp-fb:104 nack
a=rtcp-fb:104 nack pli
a=rtpmap:105 rtx/90000
a=fmtp:105 apt=104
a=rtpmap:106 H264/90000
a=fmtp:106 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42001f
a=rtcp-fb:106 goog-remb
a=rtcp-fb:106 transport-cc
a=rtcp-fb:106 ccm fir
a=rtcp-fb:106 nack
a=rtcp-fb:106 nack pli
a=rtpmap:107 rtx/90000
a=fmtp:107 apt=106
a=rtpmap:108
H264/90000
a=fmtp:108 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=42001f
a=rtcp-fb:108 goog-remb
a=rtcp-fb:108 transport-cc
a=rtcp-fb:108 ccm fir
a=rtcp-fb:108 nack
a=rtcp-fb:108 nack pli
a=rtpmap:109 rtx/90000
a=fmtp:109 apt=108
a=rtpmap:127 H264/90000
a=fmtp:127 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f
a=rtcp-fb:127 goog-remb
a=rtcp-fb:127 transport-cc
a=rtcp-fb:127 ccm fir
a=rtcp-fb:127 nack
a=rtcp-fb:127 nack pli
a=rtpmap:126 rtx/90000
a=fmtp:126 apt=127
a=rtpmap:112 H264/90000
a=fmtp:112 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=42e01f
a=rtcp-fb:112 goog-remb
a=rtcp-fb:112 transport-cc
a=rtcp-fb:112 ccm fir
a=rtcp-fb:112 nack
a=rtcp-fb:112 nack pli
a=rtpmap:113 rtx/90000
a=fmtp:113 apt=112
a=extmap:5 urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id
a=extmap:6 urn:ietf:params:rtp-hdrext:sdes:repaired-rtp-stream-id
a=extmap:3 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a\=extmap:4 urn:ietf:params:rtp-hdrext:sdes:mid
a=recvonly
m=application 9 UDP/DTLS/SCTP webrtc-datachannel
c=IN IP4 0.0.0.0
a=setup:active
a=mid:2
a=sendrecv
a=sctp-port:5000
a=ice-ufrag:ZiFDSBbcTBZWsXza
a=ice-pwd:zkRSAmJQoNZZWrSNpDjImDnUZnovVsxH

LeeTeng2001 avatar Oct 17 '24 07:10 LeeTeng2001

hello, should I provide additional infos? This is a relevant issue

LeeTeng2001 avatar Oct 21 '24 03:10 LeeTeng2001

bump

LeeTeng2001 avatar Oct 31 '24 08:10 LeeTeng2001

@LeeTeng2001 I got it! The issue was that if the RTPReceiver fails during setup we would use the nil pointers incorrectly.

I was able to get a test for it, merging now :)

Sean-Der avatar Feb 12 '25 16:02 Sean-Der