Skip to content

Commit

Permalink
Fix DTLS packets do not honor configured DTLS MTU (attempt 3)
Browse files Browse the repository at this point in the history
- 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)
  • Loading branch information
ibc committed Feb 27, 2024
1 parent 7652e76 commit cb33cc3
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 47 deletions.
3 changes: 2 additions & 1 deletion worker/include/RTC/DtlsTransport.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,8 @@ namespace RTC
return this->localRole;
}
void SendApplicationData(const uint8_t* data, size_t len);
// This method must be public since it's called within an OpenSSL callback.
void SendDtlsData(const uint8_t* data, size_t len);

private:
bool IsRunning() const
Expand All @@ -173,7 +175,6 @@ namespace RTC
}
void Reset();
bool CheckStatus(int returnCode);
void SendPendingOutgoingDtlsData();
bool SetTimeout();
bool ProcessHandshake();
bool CheckRemoteFingerprint();
Expand Down
100 changes: 54 additions & 46 deletions worker/src/RTC/DtlsTransport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,32 @@ inline static void onSslInfo(const SSL* ssl, int where, int ret)
static_cast<RTC::DtlsTransport*>(SSL_get_ex_data(ssl, 0))->OnSslInfo(where, ret);
}

inline static long onSslBioOut(
BIO* bio,
int operationType,
const char* argp,
size_t len,
int /*argi*/,
long /*argl*/,
int ret,
size_t* /*processed*/)
{
long resultOfcallback = (operationType == BIO_CB_RETURN) ? static_cast<long>(ret) : 1;

// This callback is called twice for write operations:
// - First one with operationType = BIO_CB_WRITE.
// - Second one with perationType = BIO_CB_RETURN | BIO_CB_WRITE.
// We only care about the former.
if (operationType == BIO_CB_WRITE && argp && len > 0)
{
auto* dtlsTransport = reinterpret_cast<RTC::DtlsTransport*>(BIO_get_callback_arg(bio));

dtlsTransport->SendDtlsData(reinterpret_cast<const uint8_t*>(argp), len);
}

return resultOfcallback;
}

inline static unsigned int onSslDtlsTimer(SSL* /*ssl*/, unsigned int timerUs)
{
if (timerUs == 0)
Expand Down Expand Up @@ -708,15 +734,19 @@ namespace RTC
goto error;
}

SSL_set_bio(this->ssl, this->sslBioFromNetwork, this->sslBioToNetwork);

// Set the MTU so that we don't send packets that are too large with no
// fragmentation.
// TODO: This is not honored, see issue:
// https://github.com/versatica/mediasoup/issues/1100
SSL_set_mtu(this->ssl, DtlsMtu);
DTLS_set_link_mtu(this->ssl, DtlsMtu);

// We want to monitor OpenSSL write operations into our |sslBioToNetwork|
// buffer so we can immediately send those DTLS bytes (containing full DTLS
// messages, or valid DTLS fragment messages, or combination of them) to
// the endpoint, and hence we honor the configured DTLS MTU.
BIO_set_callback_ex(this->sslBioToNetwork, onSslBioOut);
BIO_set_callback_arg(this->sslBioToNetwork, reinterpret_cast<char*>(this));
SSL_set_bio(this->ssl, this->sslBioFromNetwork, this->sslBioToNetwork);

// Set callback handler for setting DTLS timer interval.
DTLS_set_timer_cb(this->ssl, onSslDtlsTimer);

Expand Down Expand Up @@ -757,7 +787,6 @@ namespace RTC
{
// Send close alert to the peer.
SSL_shutdown(this->ssl);
SendPendingOutgoingDtlsData();
}

if (this->ssl)
Expand Down Expand Up @@ -880,7 +909,6 @@ namespace RTC

SSL_set_connect_state(this->ssl);
SSL_do_handshake(this->ssl);
SendPendingOutgoingDtlsData();
SetTimeout();

break;
Expand Down Expand Up @@ -951,9 +979,6 @@ namespace RTC
// Must call SSL_read() to process received DTLS data.
read = SSL_read(this->ssl, static_cast<void*>(DtlsTransport::sslReadBuffer), SslReadBufferSize);

// Send data if it's ready.
SendPendingOutgoingDtlsData();

// Check SSL status and return if it is bad/closed.
if (!CheckStatus(read))
{
Expand Down Expand Up @@ -1022,9 +1047,24 @@ namespace RTC
MS_WARN_TAG(
dtls, "OpenSSL SSL_write() wrote less (%d bytes) than given data (%zu bytes)", written, len);
}
}

void DtlsTransport::SendDtlsData(const uint8_t* data, size_t len)
{
MS_TRACE();

MS_DEBUG_DEV("%zu bytes of DTLS data ready to be sent", len);

// Send data.
SendPendingOutgoingDtlsData();
// Notify the listener.
this->listener->OnDtlsTransportSendData(this, data, len);

// Clear the BIO buffer.
// NOTE: Here we now that OpenSSL called the onSslBioOut() callback with
// with all the byte length that it previously wrote into the BIO mem
// buffer (this->sslBioToNetwork), so it's safe to clear the entire buffer
// after sending its entire DTLS data to the endpoint.
// NOTE: the (void) avoids the -Wunused-value warning.
(void)BIO_reset(this->sslBioToNetwork);
}

void DtlsTransport::Reset()
Expand All @@ -1044,8 +1084,9 @@ namespace RTC
this->timer->Stop();

// 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, so just don't call
// SendPendingOutgoingDTLSData().
// we don't want to send a DTLS Close Alert to the peer. However this is
// gonna happen since SSL_shutdown() will trigger a DTLS Close Alert and
// we'll have our onSslBioOut() callback called to deliver it.
SSL_shutdown(this->ssl);

this->localRole.reset();
Expand Down Expand Up @@ -1182,36 +1223,6 @@ namespace RTC
}
}

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

if (BIO_eof(this->sslBioToNetwork))
{
return;
}

int64_t read;
char* data{ nullptr };

read = BIO_get_mem_data(this->sslBioToNetwork, &data); // NOLINT

if (read <= 0)
{
return;
}

MS_DEBUG_DEV("%" PRIu64 " bytes of DTLS data ready to sent to the peer", read);

// Notify the listener.
this->listener->OnDtlsTransportSendData(
this, reinterpret_cast<uint8_t*>(data), static_cast<size_t>(read));

// Clear the BIO buffer.
// NOTE: the (void) avoids the -Wunused-value warning.
(void)BIO_reset(this->sslBioToNetwork);
}

bool DtlsTransport::SetTimeout()
{
MS_TRACE();
Expand Down Expand Up @@ -1684,9 +1695,6 @@ namespace RTC

if (ret == 1)
{
// If required, send DTLS data.
SendPendingOutgoingDtlsData();

// Set the DTLS timer again.
SetTimeout();
}
Expand Down

0 comments on commit cb33cc3

Please sign in to comment.