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

Make DTLS fragment stay within MTU size range #1156

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

ibc
Copy link
Member

@ibc ibc commented Sep 13, 2023

Details

@ibc ibc requested a review from jmillan September 13, 2023 11:29
// don't want to send a Close Alert to the peer, so just don't call
// SendPendingOutgoingDTLSData().
// NOTE: 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. However this is gonna happen.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is shown to be a problem, perhaps I can write a failing test verifying the behavior. Then do some work to try to avoid sending the Close Alert and pass the test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, could you do it once this PR is merged and try that in flatbuffers branch?

NOTE: the problem I see is that we DO WANT to send DTLS Close Alert in most of the cases, and there is just one case in which we do not want to send it (this one). So no idea this can be done. Even if we set a this->allowOutgoingDtlsCloseAlert = true flag before that scenario and set it to false immediately after ignoring such a DTLS Close Alert, the problem is that this is driven by a OpenSSL callback that probably runs asynchronously.

@ibc ibc merged commit 8472b90 into flatbuffers Sep 13, 2023
@ibc ibc deleted the make-dtls-fragment-fit-into-mtu branch September 13, 2023 12:54
ibc added a commit that referenced this pull request Feb 21, 2024
…se it causes a memory leak

- Fixes #1340
- Reopents #1100

### Details

As perfectly documented in issue #1340, there is a leak due to the usage of `BIO_set_callback_ex()` whose fix is unclear and, anyway, I'm pretty sure that the usage of that OpenSSL utility makes the performance worse.

As a workaround for issue #1100 (which now must be reopened), in order to avoid "DTLS fragment exceeds MTU size if the certificate is large", users should not pass a big DTLS certificate to mediasoup.
ibc added a commit that referenced this pull request Feb 21, 2024
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.
3 participants