search

esp-rs / esp-idf-svc

Type-Safe Rust Wrappers for various ESP-IDF services (WiFi, Network, Httpd, Logging, etc.)
https://docs.esp-rs.org/esp-idf-svc/
Apache License 2.0
287 stars 162 forks source link

Example tls_async will leak sockets #375

Open torkleyy opened 4 months ago

torkleyy commented 4 months ago

https://github.com/esp-rs/esp-idf-svc/blob/730fa3642d7042735de924c3d6ce71eafb8dc615/examples/tls_async.rs#L172

This will essentially forget the socket, thus leaking it - after a couple of dropped EspTlsSockets, we'll run into IO errors due to too many open files.

ivmarkov commented 4 months ago

Do you reproduce such a problem? Because we don't call core::mem::forget - we are simply destructuring the inner socket, so I'm not sure why you think we are forgetting anything?

torkleyy commented 4 months ago

@ivmarkov Yes, that's what happened to me.

Because we don't call core::mem::forget - we are simply destructuring the inner socket, so I'm not sure why you think we are forgetting anything?

I mean yes, but we are calling into_raw_fd which converts the socket into RawFd - which is just an integer. That means it becomes our responsibility to close the socket handle.

In my code I just changed that code to shutdown the socket and that works well for me - but I'm not sure if that's a general solution. Probably it would be enough to just take the Option to trigger the regular Drop impl.

ivmarkov commented 4 months ago

The reason why release exists in the first place is because as per my understanding, the esp idf tls socket implementation is supposed to close the socket when calling sys::esp_tls_conn_destroy. Which happens when the EspTls instance is dropped. That's essentially a hack but it exists to workaround the fact that the esp idf native tls impl thinks it owns the socket.

If anything in my chain of thought from above is incorrect, we need to revisit release.

torkleyy commented 4 months ago

I understand - in the case of esp_tls_conn_destroy, the socket does get closed. I was using esp_tls_server_session_delete (https://github.com/esp-rs/esp-idf-svc/pull/368). It seems that function is flawed, because it won't close the socket but free the tls structure... But as far as I can see in the code, it seems I can call esp_tls_conn_destroy no matter how I initialized the tls context.

Vollbrecht commented 2 weeks ago

@ivmarkov @torkleyy So since we are only using esp_tls_conn_destroy in #368 is this still a problem here?