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 #4981

Merged
merged 1 commit into from Sep 19, 2018

Conversation

Projects
None yet
3 participants
@fxposter
Copy link
Contributor

fxposter commented Jan 16, 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)
# => [1]

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.

Do not leak DNS Request IDs
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.
@fxposter

This comment has been minimized.

Copy link
Contributor Author

fxposter commented Jan 16, 2018

We can cover the problematic behavior in spect, but it requires either having a working DNS server to run the tests (ie: move the example above into a spec) or adding a spec for internal classes/methods (not sure if it would be useful, though)

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Jan 16, 2018

@fxposter have you tried this against MRI? This code is theirs. If it affects them then we should get this moved upstream.

@fxposter

This comment has been minimized.

Copy link
Contributor Author

fxposter commented Jan 16, 2018

This is not strictly true. The commit mentioned above diverges from the MRI behavior which don't have any IPAddr.new calls in this place: https://github.com/ruby/ruby/blob/trunk/lib/resolv.rb#L768.

Hence they don't leak request ids here.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Jan 17, 2018

@fxposter oh sorry I should have checked. See: jruby/ruby@44fe41b#diff-5eaee83b511bb5c304d686f8f47dc6c2L773

That commit shows we maintain doing that in both sender and recv_reply. Note this is 2.3.x support but I should have realized you probably wanted this PR against jruby-9.1 branch and not master since master is for 2.4.x support atm (well no doubt you will want it in all the places but we have not put out jruby 9.2 yet).

Could I ask two favors?

  1. Submit this against jruby-9.1 branch (the source of resolv.rb has changed a tiny bit) and make sure things are cool still. I will merge this onto master so 2.4 support works as well as update our stdlib patching which happens outside of jruby itself.
  2. See if you can observe a leak on recv_reply since we do the same thing there.

The great thing about this work is fixing up part of our MRI stdlib changeset. One less difference is one less headache at some point in the future :)

@kares

This comment has been minimized.

Copy link
Member

kares commented May 20, 2018

this has been stale for a while but seems that this makes sense to go into 9.2 ... marking for now

@kares kares added this to the JRuby 9.2.0.0 milestone May 20, 2018

@enebo enebo modified the milestones: JRuby 9.2.0.0, JRuby 9.2.1.0 May 24, 2018

@enebo enebo merged commit f0059e6 into jruby:master Sep 19, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.