Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Example tls_async will leak sockets #375

Open
torkleyy opened this issue Feb 26, 2024 · 5 comments
Open

Example tls_async will leak sockets #375

torkleyy opened this issue Feb 26, 2024 · 5 comments
Labels
question Further information is requested

Comments

@torkleyy
Copy link
Contributor

socket.into_inner().unwrap().into_raw_fd();

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
Copy link
Collaborator

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
Copy link
Contributor Author

@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
Copy link
Collaborator

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
Copy link
Contributor Author

I understand - in the case of esp_tls_conn_destroy, the socket does get closed. I was using esp_tls_server_session_delete (#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
Copy link
Collaborator

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

@Vollbrecht Vollbrecht added the question Further information is requested label Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
Status: Todo
Development

No branches or pull requests

3 participants