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/make dtls fragment stay within mtu size range #1143

Closed
wants to merge 1 commit into from

Conversation

pnts-se
Copy link

@pnts-se pnts-se commented Aug 25, 2023

closes #1100

This PR will make DTLS fragment stay within MTU size range. Currently that's not always the case, e.g. on large certificate size.

The solution is to stop calling BIO_get_mem_data() manually for pending out going data, and instead rely on a callback function, set with BIO_set_callback_ex().

Packet capture during usage when running a worker build from this branch:
mediasoup_dtls_wireshark-230825

@ibc ibc requested review from ibc and jmillan August 28, 2023 11:25
@ibc
Copy link
Member

ibc commented Aug 28, 2023

Amazing. Thanks a lot.

However, probably it will take several days before we can review and merge this PR (super busy days after summer and lot of pending work).

@pnts-se
Copy link
Author

pnts-se commented Aug 29, 2023

However, probably it will take several days before we can review and merge this PR

Yes of course, I'm in no hurry. I will try to find something else mediasoup-related to help out with.

As for this PR: I'm just beginning to learn the code base and I'm not sure if I got it right in terms of encapsulation and data hiding. Might want to refactor that.

@ibc
Copy link
Member

ibc commented Sep 13, 2023

I'm testing this PR right now, but I'll do in the flatbuffers branch to avoid conflicts once that is merged.

@ibc
Copy link
Member

ibc commented Sep 13, 2023

I see a potential problem:

	void DtlsTransport::Reset()
	{
		MS_TRACE();

		int ret;

		if (!IsRunning())
			return;

		MS_WARN_TAG(dtls, "resetting DTLS transport");

		// Stop the DTLS timer.
		this->timer->Stop();

		// We need to reset the SSL instance so we need to "shutdown" it, but we
		// don't want to send a Close Alert to the peer, so just don't call
		// SendPendingOutgoingDTLSData().
		SSL_shutdown(this->ssl);

		this->localRole.reset();
		this->state            = DtlsState::NEW;
		this->handshakeDone    = false;
		this->handshakeDoneNow = false;

		// Reset SSL status.
		// NOTE: For this to properly work, SSL_shutdown() must be called before.
		// NOTE: This may fail if not enough DTLS handshake data has been received,
		// but we don't care so just clear the error queue.
		ret = SSL_clear(this->ssl);

		if (ret == 0)
			ERR_clear_error();
	}

Note the:

// We need to reset the SSL instance so we need to "shutdown" it, but we// don't want to send a Close Alert to the peer, so just don't call
// SendPendingOutgoingDTLSData().
SSL_shutdown(this->ssl);

However in your PR we no longer call SendPendingOutgoingDTLSData() or any other method to send data. Instead it is done automatically by the sslBioOutCallback callback. The idea is that we reset mediasoup side DTLS without sending DTLS Close Alert to the endpoint and I'm afraid we are loosing this.

ibc added a commit that referenced this pull request Sep 13, 2023
@ibc
Copy link
Member

ibc commented Sep 13, 2023

I see a potential problem:

	void DtlsTransport::Reset()
	{
		MS_TRACE();

		int ret;

		if (!IsRunning())
			return;

		MS_WARN_TAG(dtls, "resetting DTLS transport");

		// Stop the DTLS timer.
		this->timer->Stop();

		// We need to reset the SSL instance so we need to "shutdown" it, but we
		// don't want to send a Close Alert to the peer, so just don't call
		// SendPendingOutgoingDTLSData().
		SSL_shutdown(this->ssl);

		this->localRole.reset();
		this->state            = DtlsState::NEW;
		this->handshakeDone    = false;
		this->handshakeDoneNow = false;

		// Reset SSL status.
		// NOTE: For this to properly work, SSL_shutdown() must be called before.
		// NOTE: This may fail if not enough DTLS handshake data has been received,
		// but we don't care so just clear the error queue.
		ret = SSL_clear(this->ssl);

		if (ret == 0)
			ERR_clear_error();
	}

Note the:

// We need to reset the SSL instance so we need to "shutdown" it, but we// don't want to send a Close Alert to the peer, so just don't call
// SendPendingOutgoingDTLSData().
SSL_shutdown(this->ssl);

However in your PR we no longer call SendPendingOutgoingDTLSData() or any other method to send data. Instead it is done automatically by the sslBioOutCallback callback. The idea is that we reset mediasoup side DTLS without sending DTLS Close Alert to the endpoint and I'm afraid we are loosing this.

Let's ignore this use case since AFAIR it never worked fine. This is, if DtlsTransport::Reset() is called once the DTLS session was established, it never works again, and probably it's what the spec says. So whether we send a DTLS Close Alert or not is irrelevant because:

  1. In most of the cases we do want to send it (when we really close/reset the DtlsTransport due to a fatal failure).
  2. The use case of reseting the DTLS session once established is not a real/needed use case.

@ibc
Copy link
Member

ibc commented Sep 13, 2023

@pnts-se I'm gonna close this PR in favour of this one #1156 which is basically a clone of yours but it targets flatbuffers branch instead of v3 branch (to avoid complex git conflicts).

Thanks a lot!

@ibc ibc closed this Sep 13, 2023
ibc added a commit that referenced this pull request Feb 27, 2024
- Fixes #1100
- Based on PR #1156 which was based on PR #1143

### Details

This PR is the same as PR #1156. However that PR introduced a memory leak (see issue #1340). This PR fixes that leak by following the discussion and research in associated issues and PRs, specially here: #1340 (comment)
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.

DTLS fragment exceeds MTU size if the certificate is large.
2 participants