versatica / mediasoup

Cutting Edge WebRTC Video Conferencing
https://mediasoup.org
ISC License
6.18k stars 1.12k forks source link

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

Closed ibc closed 7 months ago

ibc commented 7 months ago

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: https://github.com/versatica/mediasoup/issues/1340#issuecomment-1965375082

To prove that this PR works at DTLS level, here the DTLS messages sent from mediasoup to browser while doing the DTLS handshake:

RTC::DtlsTransport::SendDtlsData() | 1349 bytes of DTLS data ready to be sent
RTC::DtlsTransport::SendDtlsData() | 1349 bytes of DTLS data ready to be sent
RTC::DtlsTransport::SendDtlsData() | 1349 bytes of DTLS data ready to be sent
RTC::DtlsTransport::SendDtlsData() | 1349 bytes of DTLS data ready to be sent
RTC::DtlsTransport::SendDtlsData() | 1349 bytes of DTLS data ready to be sent
RTC::DtlsTransport::SendDtlsData() | 242 bytes of DTLS data ready to be sent
RTC::DtlsTransport::SendDtlsData() | 67 bytes of DTLS data ready to be sent
RTC::DtlsTransport::SendDtlsData() | 129 bytes of DTLS data ready to be sent
RTC::DtlsTransport::SendDtlsData() | 385 bytes of DTLS data ready to be sent
RTC::DtlsTransport::SendDtlsData() | 45 bytes of DTLS data ready to be sent

which clearly corresponds to what we see in Wireshark (there is a huge certificate so it's splitted into N DTLS Certificate fragments):

CleanShot-2024-02-27-at-16 51 19@2x

Note that sizes of each message match the DTLS data sent by mediasoup plus UDP and IP encapsulation (32 bytes always).

ibc commented 7 months ago

CC @gkrol-katmai @pnts-se

gkrol-katmai commented 7 months ago

The code looks good to me! I think we now have a good grasp of what's going on. I know that adding the 'reset' in that function will fix the leak. I haven't tested this PR.

Not sure what the coding standards are for comments, but I would suggest we add comments to onSslBioOut and DtlsTransport::SendDtlsData (above the implementation). Those should describe the effect of the function and when/why it gets called. Preferably /** */ style. We're doing some subtle things here so we need to be extra clear.

ibc commented 7 months ago

I know that adding the 'reset' in that function will fix the leak. I haven't tested this PR.

I think this is also proven here https://github.com/versatica/mediasoup/issues/1340#issuecomment-1966393672 where it's clear that the onSslBioOut is always called with len matching the previous mem_write() call.

Not sure what the coding standards are for comments, but I would suggest we add comments to onSslBioOut and DtlsTransport::SendDtlsData (above the implementation). Those should describe the effect of the function and when/why it gets called. Preferably /** */ style. We're doing some subtle things here so we need to be extra clear.

Done here: https://github.com/versatica/mediasoup/pull/1345/commits/e827248427956f53c442ae1a930700f213d9edb4

ibc commented 7 months ago

BTW I've added some logs and Wireshark capture in the PR description above proving that everything makes sense.

ibc commented 7 months ago

Let's merge and release. Thanks a lot.