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.

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

Fix checkNegotiationNeeded panic after ReplaceTrack(nil) #2561

Merged

Conversation

aalekseevx
Copy link
Member

Description

I have encountered the following application panic with peerConnection using OnNegotiationNeeded handler:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x1677cea]

goroutine 1346 [running]:
github.com/pion/webrtc/v3.(*PeerConnection).checkNegotiationNeeded(0xc0006bb680)
        /-S/vendor/github.com/pion/webrtc/v3/peerconnection.go:403 +0x16ca
github.com/pion/webrtc/v3.(*PeerConnection).negotiationNeededOp(0xc0006bb680)
        /-S/vendor/github.com/pion/webrtc/v3/peerconnection.go:342 +0x10e
github.com/pion/webrtc/v3.(*operations).start(0xc0017fb398)
        /-S/vendor/github.com/pion/webrtc/v3/operations.go:93 +0x63
created by github.com/pion/webrtc/v3.(*operations).Enqueue
        /-S/vendor/github.com/pion/webrtc/v3/operations.go:41 +0x1e5

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
peerconnection.go 100.00%

📢 Thoughts on this report? Let us know!.

@Sean-Der
Copy link
Member

Sean-Der commented Sep 7, 2023

LGTM @aalekseevx mind fixing commit message, then we can merge!

@aalekseevx aalekseevx force-pushed the fix-check-negotiation-needed-panic branch from d9d4ed1 to df0baf5 Compare September 7, 2023 06:05
@aalekseevx
Copy link
Member Author

@Sean-Der, updated!

replaceTrack with nil would cause a crash
@Sean-Der Sean-Der force-pushed the fix-check-negotiation-needed-panic branch from 701f93d to e7b9b07 Compare September 8, 2023 17:50
@Sean-Der Sean-Der merged commit e507d46 into pion:master Sep 8, 2023
18 checks passed
@aalekseevx
Copy link
Member Author

@Sean-Der, I believe this patch is worth porting to v3, what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants