net_would_block() does not use the mbedtls context for timeouts (IDFGH-14247)

[amrsoll]

Answers checklist.

• I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
• I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
• I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

5.1.5 & master

Espressif SoC revision.

ESP32s3

Operating System used.

Linux

How did you build your project?

Command line with idf.py

If you are using Windows, please specify command line type.

None

Development Kit.

Custom

Power Supply used.

Battery

What is the expected behavior?

We are writing a cross-platform library and would like to rely on network socket timeout instead of using mbedtls_net_recv_timeout().

It is expected that mbedtls_ssl_read() returns MBEDTLS_ERR_NET_RECV_FAILED when there is a signal that interrupts the connection (like the socket timeout).

What is the actual behavior?

mbedtls_ssl_read() returns MBEDTLS_ERR_SSL_WANT_READ because net_would_block() checks errno=EAGAIN and returns 1.

Steps to reproduce.

1. Step
2. Step
3. Step
   ...

Debug Logs.

No response

More Information.

Traditionally, the function int net_would_block( const mbedtls_net_context *ctx ) returns 0 when the network socket is blocking (including when there is a timeout on it).

Given that the upstream library does not behave that way, this makes cross-platform support harder.

Some references:

• [TW#12156] 'mbedtls_net_recv' timeout #434
• mbedtls and detecting non-blocking reads during handshaking #424

This commit removes the upstream behaviour. I could not find the merge request for more details, but this is the merge message from the git logs.

commit 27a4802f923c4aaf7b8730e7a1b36ba72d773e2d
Merge: c4fddc8590 87c3decc12
Author: Angus Gratton <angus@espressif.com>
Date:   Mon Oct 21 15:33:59 2019 +0800

    Merge branch 'bugfix/remove_mbedtls_would_block' into 'master'
    
    Remove check for would_block in mbedtls
    
    See merge request espressif/esp-idf!6384

This change seems incorrect, as the sockets with receive timeouts DO block until the timeout elapses. The commit message seems to be confused, as a socket with timeout should NOT meet the condition NON_BLOCK.

This is the way we are handling it currently, using a define flag CHECK_BLOCKING_SOCKET

Simplified code:

int tls_transport_connect(struct mbedtls_context *ctx, const char *hostname, const char *port)
{
	// Other mbedtls_ssl_* configuration ...

	// Not using mbedtls_net_recv_timeout, as we will rely on socket timeout instead
	mbedtls_ssl_set_bio(ssl_ctx, net_ctx, mbedtls_net_send, mbedtls_net_recv, NULL);
	// do some verifications
	return 0;
}

int tls_transport_recv(struct mbedtls_context *ctx, unsigned char *buf, size_t len)
{
	int ret = MBEDTLS_ERR_SSL_WANT_READ ;
	while (ret == MBEDTLS_ERR_SSL_WANT_READ || ret == MBEDTLS_ERR_SSL_WANT_WRITE) {
		ret = mbedtls_ssl_read(&ctx->ssl_ctx, buf, len);
#ifdef CHECK_BLOCKING_SOCKET
		/* "patching" undesired behaviour from esp-idf mbedtls.
			See https://github.com/espressif/esp-idf/commit/87c3decc1255edced1348841866b49324533d152
		*/
		if (ret == MBEDTLS_ERR_SSL_TIMEOUT || ((ret == MBEDTLS_ERR_SSL_WANT_READ || ret == MBEDTLS_ERR_NET_RECV_FAILED) && errno == EAGAIN))
#else
		if (ret == MBEDTLS_ERR_SSL_TIMEOUT  || (ret == MBEDTLS_ERR_NET_RECV_FAILED && errno == EAGAIN))
#endif
		{
			/* Timeout occured, not an error */
			skylarklive_log(SKYLARKLIVE_LOG_DBG, "mbedtls_ssl_read timeout 0x%x", ret);
			return 0;
		} else {
			// other error cases... 
		}
	}
	return ret;
}


int mqtt_read(Network *n, unsigned char *buffer, int len, int timeout_ms) {
	setsockopt(ctx->tls_ctx.net_ctx.fd, SOL_SOCKET, SO_RCVTIMEO, (char *)&interval, sizeof(struct timeval));
	bytes = 0;
	while (bytes < len) {
		rc = tls_transport_recv(&ctx->tls_ctx, &buffer[bytes], (size_t)(len - bytes));
		if (rc < 0) {
			/* Error */
			bytes = -1;
			break;
		} else if (rc == 0) {
			/* Timeout or more data to read */
			bytes = 0;
			break;
		} else {
			/* Add read bytes */
			bytes += rc;
		}
	}
}

@MTsimon