Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: rtcp interceptor nil panic #2930

Merged
merged 1 commit into from
Feb 12, 2025
Merged

fix: rtcp interceptor nil panic #2930

merged 1 commit into from
Feb 12, 2025

Conversation

LeeTeng2001
Copy link
Contributor

Description

Add a nil rtpinterceptor check before invoking read method

Reference issue

Fixes #2929

@Sean-Der
Copy link
Member

Thank you @LeeTeng2001!

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

@codecov Codecov
Copy link

codecov bot commented Oct 14, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

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%) ⬆️
wasm 63.24% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LeeTeng2001
Copy link
Contributor Author

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
Copy link
Contributor Author

LeeTeng2001 commented Oct 14, 2024 •
edited
Loading

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()
}

@Sean-Der
Copy link
Member

@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

@LeeTeng2001
Copy link
Contributor Author

LeeTeng2001 commented Oct 16, 2024 •
edited
Loading

@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
Copy link
Contributor Author

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

@LeeTeng2001
Copy link
Contributor Author

LeeTeng2001 commented Oct 16, 2024 •
edited
Loading

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

@Sean-Der
Copy link
Member

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

I just want to understand/fix the root cause

@LeeTeng2001
Copy link
Contributor Author

LeeTeng2001 commented Oct 17, 2024 •
edited
Loading

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
Copy link
Contributor Author

LeeTeng2001 commented Oct 21, 2024 •
edited
Loading

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

@LeeTeng2001
Copy link
Contributor Author

bump

@Sean-Der Sean-Der force-pushed the master branch 2 times, most recently from e057683 to 5b8323d Compare February 12, 2025 16:15
@Sean-Der
Copy link
Member

@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 :)

@LeeTeng2001 @Sean-Der
Fix error handling in RTPReceiver.Receive
7244222 
If we failed to startReceive we would still make the Receiver as ready
to start reading.

Fixes pion#2929
@Sean-Der Sean-Der merged commit 306dc37 into pion:master Feb 12, 2025
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nil pointer dereference in RTPReceiver readRTP
2 participants
@LeeTeng2001 @Sean-Der