alexdalitz / dnsruby

Dnsruby is a feature-complete DNS(SEC) client for Ruby, as used by many of the world's largest DNS registries and the OpenDNSSEC project
Other
197 stars 77 forks source link

The Additional section of Dnsruby::Message sometimes returns false when compared to itself. #57

Closed keithrbennett closed 9 years ago

keithrbennett commented 9 years ago

The Additional section of Dnsruby::Message sometimes returns false when compared to itself, i.e.

(a_message.additional == a_message.additional) => false

This results in the entire message when compared to itself returning false:

(a_message == a_message) => false

I think it has something to do with the contained RRSet's and/or RR's; the act of comparing them may return new objects who don't have an == method defined, resulting in == returning false for different instances even though they are semantically equal.

A script illustrating this is at https://gist.github.com/keithrbennett/469afd51f7fd075fdc5a , and also pasted here:

#!/usr/bin/env ruby

# keithrbennett, 2015-01-06
#
# Illustrates that the Additional section of Dnsruby::Message
# sometimes returns false when compared to itself, i.e.
# (a_message.additional == a_message.additional) => false
#
# which results in the entire message when compared to itself returning false:
# (a_message == a_message) => false

# I think it has something to do with the contained RRSet's and/or RR's;
# the act of comparing them may return new objects who don't have an ==
# method defined, resulting in == returning false for different instances
# even though they are semantically equal.

require 'dnsruby'
include Dnsruby

def response_from_network
  query = Message.new('cnn.com', 'NS')
  Resolver.new.send_message(query)
end

def response_saved
  response_as_string = "\x10\a\x81\x90\x00\x01\x00\x04\x00\x00\x00\x06\x03cnn\x03com\x00\x00\x02\x00\x01\xC0\f\x00\x02\x00\x01\x00\x01QC\x00\x14\x03ns3\ntimewarner\x03net\x00\xC0\f\x00\x02\x00\x01\x00\x01QC\x00\x11\x03ns2\x03p42\x06dynect\xC04\xC0\f\x00\x02\x00\x01\x00\x01QC\x00\x06\x03ns1\xC0)\xC0\f\x00\x02\x00\x01\x00\x01QC\x00\x06\x03ns1\xC0I\xC0%\x00\x01\x00\x01\x00\x001\xA2\x00\x04\xC7\aD\xEE\xC0E\x00\x01\x00\x01\x00\x00\xB1\x0E\x00\x04\xCC\r\xFA*\xC0b\x00\x01\x00\x01\x00\x009`\x00\x04\xCCJl\xEE\xC0t\x00\x01\x00\x01\x00\x00\xBDg\x00\x04\xD0NF*\xC0t\x00\x1C\x00\x01\x00\x00\x00\xBB\x00\x10 \x01\x05\x00\x00\x90\x00\x01\x00\x00\x00\x00\x00\x00\x00B\x00\x00)\x0F\xA0\x00\x00\x80\x00\x00\x00".force_encoding("ASCII-8BIT")
  Message.decode(response_as_string)
end

def report_header
  "%7s  %s\n#{'-' * 40}" % %w(T/F  Object)
end

def report(term, object)
  puts("%7s  %s" % [object == object, term])
end

r = response_saved

# Uncomment this if you want to test a real network call:
# r = response_from_network

puts report_header
report('response', r)
report('response.header', r.header)
report('response.question', r.question)
report('response.answer', r.answer)
report('response.additional', r.additional)

=begin

Outputs:

   T/F  Object
----------------------------------------
  false  response
   true  response.header
   true  response.question
   true  response.answer
  false  response.additional

=end
alexdalitz commented 9 years ago

Thanks for the report! I've had a quick look, and the problem seems to be the OPT record. If you add the OPT record from the additional section to the answer section, then the answer section also displays this behaviour. I'll investigate some more.

Thanks!

Alex.

alexdalitz commented 9 years ago

Commit 9279032 should fix this problem. I've simply removed the OPT record from the comparison for equality

keithrbennett commented 9 years ago

This addresses the use case I reported, but I fear that we are not addressing the underlying problem, which could pop up in other ways that might be difficult to diagnose and deal with. Also, if there are OPT records, shouldn't they be included in the comparison for equality?

I just ran the test script against the commit in Github whose hash is bc7d6e0 (bc7d6e082c48cbc3533604cf237b5f914de36a2b), dated 9/21/2007, labelled "Initial dnsruby code import to rubyforge" and got the same result.

It would be nice to delve more deeply into this and find the root cause of the issue. If you don't have time to do so, could you reopen this issue to remind us that it is not fully resolved?

Thanks, Keith

alexdalitz commented 9 years ago

Well, the OPT record is not really part of the Message, but conveys metadata about the message transport - this is why it is not displayed on dig output, for example. It doesn't seem unreasonable to exclude it from message comparison, and this is the behaviour which dnsruby has traditionally displayed.

I'd be interested to hear of any issues as a result, and would also note that it's possible to compare the Message including the OPT record, if it's required by an application.