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

STUN Server null reference exception fix #1337

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

ispysoftware
Copy link
Contributor

If you add a TURN option to ice it sends allocate requests to the STUN server which results in null reference exceptions as the STUN server only accepts binding requests. This adds a more helpful error message to debug output.

use wyze camera name from url
Add timeout to ice gathering
Add prefer H264 flag for compatible formats
Add sanity check for ICE Gathering timeout
Add sanity check for ICE Gathering timeout
Fix bug in g722 codec
Fix bug where duplicate durations were being added to local track timestamp in SendAudioFrame
Ignore H264 formats that use unsupported packetization modes
Clean up logic in AreMatch
@sipsorcery
Copy link
Member

What's actually changing in this PR? Which line has the change and is not just a refactor?

@ispysoftware
Copy link
Contributor Author

ispysoftware commented Feb 24, 2025

It's not returning null..when it returned null you got a null reference exception.

@sipsorcery
Copy link
Member

Is this the non-refactor change?

default://Allocate not supported - this is a STUN server only not a full TURN server
    throw new ApplicationException("Unsupported MessageType " + stunRequest.Header.MessageType);

Is there any reason to throw an exception? With protocols like STUN & RTCP there are regularly unsupported message types. In 99% of cases it's safe to ignore them and allow the connection to proceed. Throwing an exception will potentially mean WebRTC connections will be blocked by an unknown STUN message type.

@ispysoftware
Copy link
Contributor Author

ispysoftware commented Feb 25, 2025

I only found two references to it in the code base and there was no null check on the result hence the null reference exception. You make a good point though it should probably be fixed upstream.

@ispysoftware
Copy link
Contributor Author

ispysoftware commented Feb 26, 2025

Actually.. The method i changed is private and it's only called in 2 locations in SipSorcery code, both of which will throw a null reference exception immediately afterwards if GetResponse returns null. I think it's better design to throw a meaningful exception message rather than let it fail with a null reference exception which is basically meaningless to consumers.

STUNMessage stunResponse = GetResponse(receivedEndPoint, stunRequest, true);
byte[] stunResponseBuffer = stunResponse.ToByteBuffer(null, false);

@ispysoftware ispysoftware reopened this Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants