Skip to content

DTLSTransport: Possible ThreadPool starvation with MbedTLS #1597

Description

@ts-numfum

I noticed strange behavior while I was testing libdatachannel with TURN servers using MbedTLS.

I followed the copy-paste example to create a datachannel and send messages back and forth.
The difference in my version was that both the answerer and the offerer was running in the same process, but on different threads and they handled the exchange of session descriptions and ice candidates automatically by message passing.

I wanted to test if libdatachannel can handle creating multiple peer connections and data channels in parallel, so I executed the same test, but now 5 data channels were being created in parallel. Everything looked OK.

But then I enabled TransportPolicy::Relayed and forced the connections to use our TURN servers, and this is where the problems started.

The connections got stuck after the RtcIceState::Connected event and by enabling the MbedTLS debug callback, I could see that the DTLS handshake always failed because data was missing, and the data never arrived, so the handshake got rescheduled over and over again.

The problem was with this code:

int ret;
{
	std::lock_guard lock(mSslMutex);
	ret = mbedtls_ssl_handshake(&mSsl);
}

if (ret == MBEDTLS_ERR_SSL_WANT_READ) {
	ThreadPool::Instance().schedule(mTimerSetAt + milliseconds(mFinMs),
	                                [weak_this = weak_from_this()]() {
		                                if (auto locked = weak_this.lock())
			                                locked->doRecv();
	                                });
	return;
}

If the timer is not updated inside mbedtls_ssl_handshake or if it's in a cancelled state , then mTimerSetAt will contain an old time point and mFinMs could be 0, so schedule puts the task to the front of the priority queue (because it is scheduled into the past).

Because the client runs on the same threadpool, the ClientHello message will never arrive because the send task is never executed.

I propose:

  • Use milliseconds(mFinMs ? mFinMs : MBEDTLS_SSL_DTLS_TIMEOUT_DFL_MIN) instead of mTimerSetAt + milliseconds(mFinMs) to ensure that the task is scheduled reasonably
  • Make the auto ThreadPool::schedule(clock::time_point time, F &&f, Args &&...args) noexcept method private on the ThreadPool and keep only the overload with clock::duration public to avoid issues like this in the future.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions