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

Do not leak DNS Request IDs #5019

Conversation

@olleolleolle
Copy link
Member

@olleolleolle olleolleolle commented Jan 29, 2018

(This is a cherry-pick of #4981, here against jruby-9.1.)


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.

While jruby@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.
@olleolleolle
Copy link
Member Author

@olleolleolle olleolleolle commented Jan 29, 2018

@enebo This is step 1 in the #4981 PR.

@enebo enebo added this to the JRuby 9.1.16.0 milestone Jan 29, 2018
@enebo enebo merged commit 26005d0 into jruby:jruby-9.1 Jan 29, 2018
1 check failed
@enebo
Copy link
Member

@enebo enebo commented Jan 29, 2018

@olleolleolle @headius I think we will also need to update our MRI stdlib with this commit. I guess though there will be a collection of PRs on this by the time it is done.

@olleolleolle olleolleolle deleted the carry-do-not-leak-dns-request-ids-to-jruby-9-1 branch Jan 29, 2018
@olleolleolle
Copy link
Member Author

@olleolleolle olleolleolle commented Jan 30, 2018

Noting the progress: the above update was done in f8ac719

jsvd
Copy link

jsvd commented on ada05fa Jun 8, 2018

This seems to have been reverted later in c95943a#diff-6d2422fab9324bf20363957b293b3658
@olleolleolle do you know if the leak is back after this revert? Is it solved instead by #5074?

olleolleolle
Copy link
Member

olleolleolle commented on ada05fa Jun 8, 2018

I'm just reading code here, but it seems to me that these lines https://github.com/quixoten/jruby/blob/987406f67aa114e2c1a2cc5cf8d59b91a9a18e97/lib/ruby/stdlib/resolv.rb#L790-L791 are in there (looking at the code in #5074)

          service = [IPAddr.new(host), port]
          id = DNS.allocate_request_id(service[0], service[1])
jsvd
Copy link

jsvd commented on ada05fa Jun 8, 2018

right, that PR added it back again :) thanks for responding!

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

4 participants