use same parameter for DNS.allocate_request_id/free_request_id to fix leak#5722
use same parameter for DNS.allocate_request_id/free_request_id to fix leak#5722headius merged 1 commit intojruby:masterfrom
Conversation
|
I just checked in upstream Ruby and it actually does not have this problem. |
|
@colinsurprenant Ahh thanks for the footwork on this. We need to sort out what's the correct logic and do our best to sync it up between JRuby and CRuby. |
|
@colinsurprenant Oh, so...if we pull CRuby's actual resolv.rb unmodified are we reintroducing that other issue? |
|
@colinsurprenant Ah yes, @enebo refreshed my memory. So I believe #4496 was because our IPAddr and related structures were not quite matching CRuby...just another part of the complicated socket subsystem that we're not emulating exactly right. But things have changed in the meantime and a whole bunch of socket compat fixes got merged in within the last two years...we might be ok? |
|
@headius sounds good, I can certainly take a look! |
|
@colinsurprenant Any status update on this? I don't have a problem merging this to fix JRuby but it would obviously be nicer if we could move toward MRI's version rather than further away. |
|
Dropped the ball on this :( |
|
@colinsurprenant Ping again. 😁 I'm still unclear on whether we should merge this or update to a different version of MRI's resolv.rb (or whether we might already have the best version due to the stdlib 2.7 update going into 9.2.9.) |
|
Updating some details on my own... The patch that @colinsurprenant proposes here does make our resolv.rb diverge further from the CRuby version. However the bug is caused by a separate divergence, and this just fixes that patch to work better. If I'm understanding correctly, the change introduced in #4496 causes this to break because the So I think the patch is is fine; the fix in #4496 was incomplete in that it only used the processed host along one path. |
This patch is from jruby/jruby#5722, which fixes some missed logic in the patch from jruby/jruby#4496.
3 participants
As described in logstash-plugins/logstash-filter-dns#52
resolv.rbwas updated from upstream Ruby stdlib starting at JRuby 9.2.0.0 which essentially re-introduced issue logstash-plugins/logstash-filter-dns#40 where all DNS request will eventually systematically time out.This problem will only be triggered if there are multiple nameserver hosts specified OR if there is no nameserver host but there are more than one host in
/etc/resolv.confresulting in the usage of theUnconnectedUDPclass inresolv.rb.Essentially the problem is that the
DNS.allocate_request_id(host, port)call will cache the allocated id using thehostandportparameters.But the cache cleanup will be done in the
closemethod by the DNS.free_request_id(service[0], service[1], id) call where theservice[0]will beIPAddr.new(host)and not the originalhoststring used in thesendermethod leading to not finding the original cache key and not freeing it from the cache which is 64k elements wide and once full, theallocate_request_idmethod will loop forever.This fix just makes sure that the exact same parameters are used in both
DNS.allocate_request_idandDNS.free_request_idusing theIPAddr#to_swhich will always yield the same host string.I am planning on submitting this issue upstream in Ruby but I figured I'd also submit here to provide a potential quicker turnaround for this nasty bug.