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

Fix file descriptor leak in resolv.rb #5074

Merged
merged 5 commits into from Mar 20, 2018
Merged

Conversation

@quixoten
Copy link
Contributor

@quixoten quixoten commented Mar 1, 2018

There's an edge case in resolv.rb that can lead to file descriptor leaks on a highly saturated system (logstash in our case).

file-descriptor-leak

(the cliff at the end is from a restart of logstash)

We're using the logstash-filter-dns plugin which uses timeout.

When a timeout fires while waiting for a random port, the new object is never returned to the caller. Instead requester is set to nil. There's no handle to any sockets that were created in the initializer, so they can't be closed, and one ore more file descriptors is leaked.

@headius
Copy link
Member

@headius headius commented Mar 2, 2018

Great find! We just ship CRuby's resolv.rb...so it seems likely they'd be affected too. I'll have a look at your patches.

@headius
Copy link
Member

@headius headius commented Mar 2, 2018

Ok yeah, I am pretty sure CRuby would be affected by this as well. We need to submit this to them as a patch with backports to 2.3.

@quixoten
Copy link
Contributor Author

@quixoten quixoten commented Mar 2, 2018

I'll work on getting it submitted to CRuby.

@quixoten
Copy link
Contributor Author

@quixoten quixoten commented Mar 2, 2018

@quixoten
Copy link
Contributor Author

@quixoten quixoten commented Mar 7, 2018

The patch was accepted into CRuby. Should this be closed now?

@headius
Copy link
Member

@headius headius commented Mar 20, 2018

We'll merge it in, thanks!

@headius
Copy link
Member

@headius headius commented Mar 20, 2018

It doesn't appear they have committed this to their 2.5 branch yet, so I'll merge to our fork for now.

@headius headius merged commit 3813f0a into jruby:master Mar 20, 2018
1 check failed
headius added a commit to jruby/ruby that referenced this issue Mar 20, 2018
headius added a commit to jruby/ruby that referenced this issue Mar 20, 2018
@headius headius added this to the JRuby 9.2.0.0 milestone Mar 21, 2018
@headius
Copy link
Member

@headius headius commented Mar 21, 2018

Once there's a backport for 2.3 we can add this to JRuby 9.1.17.

jsvd referenced this issue Jun 8, 2018
While d1a760e fixed handling of compressed IPv6 addresses,
it also broke the "freeing" part of it.

Currently every DNS request leaks single request id:
```
require 'resolv'

Resolv::DNS::RequestID.values.map(&:length)

Resolv::DNS.new.getaddress('example.com')

Resolv::DNS::RequestID.values.map(&:length)
```

Given the fact that request ids are chosen from `0x0000..0xffff` range - the app can issue 65535 requests and then will be blocked forever trying to allocate another one.

This commit makes request id caching work by using same data for allocation and freeing.
@quixoten
Copy link
Contributor Author

@quixoten quixoten commented Jan 4, 2019

Looks like this made it into 2.3. Would it be possible to add this to a 9.1.17 release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants