-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Conversation
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 jruby#3663.
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? |
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:
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! |
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. |
@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) |
@enebo Agreed. |
Thanks. Any chance to see it backported to 1.7? |
This patch is from jruby/jruby#5722, which fixes some missed logic in the patch from jruby/jruby#4496.
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.