Fix DTLS packets do not honor configured DTLS MTU (attempt 3)

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+### Next
+
+- Fix DTLS packets do not honor configured DTLS MTU (attempt 3) ([PR #1345](https://github.com/versatica/mediasoup/pull/1345)).
+
 ### 3.13.22
 
 - Fix wrong publication of mediasoup NPM 3.13.21.

worker/include/RTC/DtlsTransport.hpp

@@ -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
@@ -173,7 +175,6 @@ namespace RTC
 		}
 		void Reset();
 		bool CheckStatus(int returnCode);
-		void SendPendingOutgoingDtlsData();
 		bool SetTimeout();
 		bool ProcessHandshake();
 		bool CheckRemoteFingerprint();

worker/src/RTC/DtlsTransport.cpp

@@ -47,15 +47,41 @@ 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 operationType = 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)
+	if (timerUs == 0u)
 	{
-		return 100000;
+		return 100000u;
 	}
-	else if (timerUs >= 4000000)
+	else if (timerUs >= 4000000u)
 	{
-		return 4000000;
+		return 4000000u;
 	}
 	else
 	{
@@ -71,15 +97,15 @@ namespace RTC
 	static constexpr int DtlsMtu{ 1350 };
 	static constexpr int SslReadBufferSize{ 65536 };
 	// AES-HMAC: http://tools.ietf.org/html/rfc3711
-	static constexpr size_t SrtpMasterKeyLength{ 16 };
-	static constexpr size_t SrtpMasterSaltLength{ 14 };
+	static constexpr size_t SrtpMasterKeyLength{ 16u };
+	static constexpr size_t SrtpMasterSaltLength{ 14u };
 	static constexpr size_t SrtpMasterLength{ SrtpMasterKeyLength + SrtpMasterSaltLength };
 	// AES-GCM: http://tools.ietf.org/html/rfc7714
-	static constexpr size_t SrtpAesGcm256MasterKeyLength{ 32 };
-	static constexpr size_t SrtpAesGcm256MasterSaltLength{ 12 };
+	static constexpr size_t SrtpAesGcm256MasterKeyLength{ 32u };
+	static constexpr size_t SrtpAesGcm256MasterSaltLength{ 12u };
 	static constexpr size_t SrtpAesGcm256MasterLength{ SrtpAesGcm256MasterKeyLength + SrtpAesGcm256MasterSaltLength };
-	static constexpr size_t SrtpAesGcm128MasterKeyLength{ 16 };
-	static constexpr size_t SrtpAesGcm128MasterSaltLength{ 12 };
+	static constexpr size_t SrtpAesGcm128MasterKeyLength{ 16u };
+	static constexpr size_t SrtpAesGcm128MasterSaltLength{ 12u };
 	static constexpr size_t SrtpAesGcm128MasterLength{ SrtpAesGcm128MasterKeyLength + SrtpAesGcm128MasterSaltLength };
 	// clang-format on
 
@@ -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);
 
@@ -757,7 +787,6 @@ namespace RTC
 		{
 			// Send close alert to the peer.
 			SSL_shutdown(this->ssl);
-			SendPendingOutgoingDtlsData();
 		}
 
 		if (this->ssl)
@@ -880,7 +909,6 @@ namespace RTC
 
 				SSL_set_connect_state(this->ssl);
 				SSL_do_handshake(this->ssl);
-				SendPendingOutgoingDtlsData();
 				SetTimeout();
 
 				break;
@@ -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))
 		{
@@ -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()
@@ -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();
@@ -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();
@@ -1684,9 +1695,6 @@ namespace RTC
 
 		if (ret == 1)
 		{
-			// If required, send DTLS data.
-			SendPendingOutgoingDtlsData();
-
 			// Set the DTLS timer again.
 			SetTimeout();
 		}