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

Use ipaddr to compare the sender's address #4496

Merged
merged 1 commit into from Mar 1, 2017

Conversation

Projects
None yet
4 participants
@nbarrientos
Contributor

nbarrientos commented Feb 19, 2017

When resolving names using compressed IPv6-only DNS servers, it could
happen that when processing the response of a name query, the IP of the
sender is not compressed so using a simple string comparison is not
safe.

This patch encapsulates the from address and the address of the sender in
IPAddr objects so the comparison is safe and just works in all cases.

This fixes #3663.

Use ipaddr to compare the sender's address
When resolving names using compressed IPv6-only DNS servers, it could
happen that when processing the response of a name query, the IP of the
sender is not compressed so using a simple string comparison is not
safe.

This patch encapsulates the from address and the address of the sender in
IPAddr objects so the comparison is safe and just works in all cases.

This fixes #3663.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 23, 2017

Member

This looks fine to me, and we'll probably merge it, but I have a few concerns.

We normally try to avoid patching the standard library if we can help it. Especially like this, where a new dependency is being introduced. But perhaps MRI has this problem too when running in an IPv6 environment?

I don't have any IPv6 setup right now. Any chance you can try to reproduce with MRI?

Member

headius commented Feb 23, 2017

This looks fine to me, and we'll probably merge it, but I have a few concerns.

We normally try to avoid patching the standard library if we can help it. Especially like this, where a new dependency is being introduced. But perhaps MRI has this problem too when running in an IPv6 environment?

I don't have any IPv6 setup right now. Any chance you can try to reproduce with MRI?

@headius headius added this to the JRuby 9.1.8.0 milestone Feb 23, 2017

@nbarrientos

This comment has been minimized.

Show comment
Hide comment
@nbarrientos

nbarrientos Feb 23, 2017

Contributor

Hi,

Thanks for looking at the patch.

I could not reproduce the problem with MRI, see this comment. But anyway:

# grep nameserver /etc/resolv.conf
nameserver 2001:1458:201:1100::5
nameserver 2001:1458:201:1100::5
# ruby -v
ruby 2.0.0p648 (2015-12-16) [x86_64-linux]
# time ruby -e "require 'resolv'; puts Resolv.getaddress 'web.cern.ch'"
188.184.9.235

real	0m0.083s
user	0m0.061s
sys	0m0.021s

Same machine, same environment, but on top of jRuby:

# jruby -v
jruby 9.1.7.0 (2.3.1) 2017-01-11 68056ae OpenJDK 64-Bit Server VM 25.121-b13 on 1.8.0_121-b13 +jit [linux-x86_64]
# time jruby -e "require 'resolv'; puts Resolv.getaddress 'web.cern.ch'"
Resolv::ResolvError: no address for web.cern.ch
  getaddress at /root/jruby-9.1.7.0/lib/ruby/stdlib/resolv.rb:100
  getaddress at /root/jruby-9.1.7.0/lib/ruby/stdlib/resolv.rb:50
      <main> at -e:1

real	2m45.023s
user	0m5.508s
sys	0m0.343s
#

I'm happy to perform other tests with MRI, just let me know :)

Do you see feasible to backport the patch to 1.7? That'd be awesome!

Contributor

nbarrientos commented Feb 23, 2017

Hi,

Thanks for looking at the patch.

I could not reproduce the problem with MRI, see this comment. But anyway:

# grep nameserver /etc/resolv.conf
nameserver 2001:1458:201:1100::5
nameserver 2001:1458:201:1100::5
# ruby -v
ruby 2.0.0p648 (2015-12-16) [x86_64-linux]
# time ruby -e "require 'resolv'; puts Resolv.getaddress 'web.cern.ch'"
188.184.9.235

real	0m0.083s
user	0m0.061s
sys	0m0.021s

Same machine, same environment, but on top of jRuby:

# jruby -v
jruby 9.1.7.0 (2.3.1) 2017-01-11 68056ae OpenJDK 64-Bit Server VM 25.121-b13 on 1.8.0_121-b13 +jit [linux-x86_64]
# time jruby -e "require 'resolv'; puts Resolv.getaddress 'web.cern.ch'"
Resolv::ResolvError: no address for web.cern.ch
  getaddress at /root/jruby-9.1.7.0/lib/ruby/stdlib/resolv.rb:100
  getaddress at /root/jruby-9.1.7.0/lib/ruby/stdlib/resolv.rb:50
      <main> at -e:1

real	2m45.023s
user	0m5.508s
sys	0m0.343s
#

I'm happy to perform other tests with MRI, just let me know :)

Do you see feasible to backport the patch to 1.7? That'd be awesome!

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 23, 2017

Member

Ok, bummer that MRI works ok here but I think we can move forward with this patch for now.

@enebo Any objections to this patch other than what I mentioned above? It would at least get people on their feet while we investigate the root cause.

Member

headius commented Feb 23, 2017

Ok, bummer that MRI works ok here but I think we can move forward with this patch for now.

@enebo Any objections to this patch other than what I mentioned above? It would at least get people on their feet while we investigate the root cause.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Feb 23, 2017

Member

@headius I think if we can open up a new issue to figure out why we are different and close the issue around this PR we can land it. Then it will work as a workaround until we dig deeper. I would rather we release sooner than later...and hey it will not be our first modification to stdlib files :|

I would like to close the original report so we can display this is now working in the release in our notes (but we can link to it)

Member

enebo commented Feb 23, 2017

@headius I think if we can open up a new issue to figure out why we are different and close the issue around this PR we can land it. Then it will work as a workaround until we dig deeper. I would rather we release sooner than later...and hey it will not be our first modification to stdlib files :|

I would like to close the original report so we can display this is now working in the release in our notes (but we can link to it)

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 1, 2017

Member

@enebo Agreed.

Member

headius commented Mar 1, 2017

@enebo Agreed.

@headius headius merged commit 6ef3f31 into jruby:master Mar 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nbarrientos

This comment has been minimized.

Show comment
Hide comment
@nbarrientos

nbarrientos Mar 2, 2017

Contributor

Thanks. Any chance to see it backported to 1.7?

Contributor

nbarrientos commented Mar 2, 2017

Thanks. Any chance to see it backported to 1.7?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment