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

Potentially leaking file descriptor that may lead to fatal crash #677

Merged
merged 2 commits into from
Apr 13, 2019

Conversation

nifly
Copy link
Contributor

@nifly nifly commented Mar 28, 2019

GCDAsyncSocket.m:1620, if createSocket fails for any reason and assigns SOCKET_NULL to socket6FD, and if socket4FD was valid, socket4FD will then be closed but without being set to SOCKET_NULL, leaving it to be closed once again later during dealloc -> closeWithError:...

If any other part of the system has acquired that file descriptor in the meantime, the application can become unstable and/or crash, often with EXC_GUARD if any other part of the system has acquired and put a guard on it.

I see this error daily in about 0.02% of my user sessions.

…crashes.

At line 1620, if createSocket fails for any reason and assigns SOCKET_NULL to socket6FD, then socket4FD will be closed but it would not be set to SOCKET_NULL, leaving it to be closed once again during dealloc... If any other part of the system has acquired that file descriptor in the meantime, the application can become unstable and/or crash (often with EXC_GUARD).
@seanm
Copy link
Contributor

seanm commented Apr 9, 2019

Both those commits look correct to me! Thanks for sharing!

@seanm
Copy link
Contributor

seanm commented Apr 11, 2019

@chrisballinger @jakebromberg @jdeff @zhouzhongguang as you've all committed to this project recently, perhaps you'd care to review this too? Who has permission to actually accept and merge PRs anyway?

Copy link
Collaborator

@chrisballinger chrisballinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks!

@chrisballinger chrisballinger merged commit 8aa7bd4 into robbiehanson:master Apr 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants