Mbed-TLS
mbedtls_net_free shouldn't close the connection
Suggested enhancement
mbedtls_net_free shouldn't close the connection, only free associated memory. Or more realistically, to avoid breaking changes, a new method which only frees memory without closing the socket should be implemented.
Justification
When you fork a process that has mbedtls_net_context objects in memory then there is no way to safely clean up the mbedtls_net_context in the child process without disrupting the connection in the parent process. I should be able to control the allocated memory in an individual process without it impacting another process.
@yanesca can you help me find someone to take a look at this issue please. While this was less of an issue during static pinned linkage, we are currently in the process of upstreaming into a project and they've requested that we dynamically link to libmedtls (v3.x.x).
I would expect that if any heap-based allocations are added to mbedtls_net_context then the forwards compatible way of that being freed would be contained in mbedtls_net_free. However, I cannot call that function, as it performs os level socket closure.
If an empty function, such as mbedtls_net_free_with_closure were to exist, then I'd be able to link dynamically in a forwards compatible way.
As a practical matter, currently (in all versions of Mbed TLS up to 3.6.x, and in 4.x as planned so far), the only resource that belongs to the mbedtls_net_context object is the file descriptor. So in practice you can just not call mbedtls_net_free() in the child process. Mbed TLS never calls mbedtls_net_free() internally since no other module in the library assumes that it's handling a connection that specifically relies on the net_sockets module.
(Although I don't really understand the use case — if the parent process is handling the connection then you should close the file descriptor in the child, and vice versa.)
I do agree that it could be useful for the API to distinguish between ownership of the file/socket descriptor and ownership of any other potential resource. As there is no urgency and this can be done in a backward-compatible way by adding a new library function, I'm adding this to the backlog, and not scheduling it for 4.0.
So in practice you can just not call
mbedtls_net_free()in the child process.
That's what I am doing while pinned.
can be done in a backward-compatible way
Is this a guarantee that no heap allocated memory will ever be added to mbedtls_net_context? The current version would be forwards incompatible due to a behavioural change that would cause memory leaks in applications against future versions of mbedtls. The assumption here being, that mbedtls_net_free should be called to clear the heap allocated memory.
I will move forward without this, but I hope you will kindly update this ticket prior to release of any such scenario.
Is this a guarantee that no heap allocated memory will ever be added to mbedtls_net_context?
No. The function's semantics is “Closes down the connection and free associated data”. Currently there is no associated data, but a future minor release may add associated data in the form of a pointer to a heap-allocated block. If that happens, mbedtls_net_free() will free that block.
We won't make this change in a 3.6 LTS patch update, because we don't change the ABI in LTS releases, and adding a pointer to mbedtls_net_context would be an ABI change.
We can, at any time, add a function that frees the associated data in an mbedtls_net_context without closing the underlying network connection. Adding a new function is always backward-compatible.
We won't make this change in a 3.6 LTS patch update
Noted.
We can, at any time, add a function that frees the associated data in an
mbedtls_net_contextwithout closing the underlying network connection. Adding a new function is always backward-compatible.
Understood, but this does mean I will need to keep a closer eye on minor releases going forward. To patch an eventual leak that could happen.
Thanks for providing the insights, I appreciate your time.