Opened 10 years ago

Closed 9 years ago

#69 closed defect (fixed)

review: data source hangs with CNAME loops

Reported by: jinmei Owned by: each
Priority: low Milestone: 05. 3rd Incremental Release: Serious Secondary
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

see datasrc_unittest.cc:CNAMELoop of rev1124 (currently commented out).

If you enable this test the data source module hangs (I suspect it will eventually stop due to memory allocation failure, but I didn't go that far).

The data source module detects the loop and reject it.

Subtickets

Change History (13)

comment:1 Changed 10 years ago by jinmei

I meant "should detect and reject it".

comment:2 Changed 10 years ago by each

  • Status changed from new to accepted

Addressed in r1301 using the same method BIND9 does: a "restarts" counter in the Query class, which is limited to 16.

(Note that while this avoids the more serious bug--the infinite loop--there's still a bug. Currently there will be 17 duplicate CNAMEs in the ANSWER section, whereas BIND9 omits duplicates.)

comment:3 follow-up: Changed 10 years ago by each

  • Owner changed from each to jinmei
  • Status changed from accepted to assigned

On further thought, I think that latter issue should probably be addressed in Message.addRRset(), so I'm going to give this ticket to Jinmei to think about...

comment:4 in reply to: ↑ 3 Changed 10 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Priority changed from critical to minor

Replying to each:

On further thought, I think that latter issue should probably be addressed in Message.addRRset(), so I'm going to give this ticket to Jinmei to think about...

I think we should rather keep Message::addRRset() dumb and have the calling apps handle such a rare, mostly pathological cases. Assuming we (eventually) do tigher validation on generating data sources, this type of duplicates should be very rare. On the other hand, since the message method is called per RRset for every response, it would be a huge waste to check this rare event in addRRset().

In fact, BIND9's message module adtops the same policy, i.e., not checking duplicate RDATA in the rendering side of code path (although I'm not sure if the performance concern was a reason for that).

I think we should revisit the data source architecture and query logic pretty substantially anyway (see https://bind10.isc.org/ticket/70#comment:12), however, it makes sense to consider this specific topic in that context, too. So I'll simply keep this ticket open as a minor issue with no assignee.

comment:5 Changed 10 years ago by shane

  • Owner changed from UnAssigned to each

comment:6 Changed 10 years ago by shane

Need to discuss on bind10-dev. Can you please send a message? Thanks!

comment:7 Changed 9 years ago by shane

  • Component changed from Unclassified to data source

comment:8 Changed 9 years ago by each

  • Owner changed from each to UnAssigned
  • Status changed from assigned to reviewing

I have a proposal to address this in r2270, could someone please review?

I've added a new method to the Message class, "hasRRset()", which returns true if the message section already contains an rrset of the same name and type as the one being added. The data source now calls this before adding rrsets to the message.

This is essentially the same mechanism BIND 9 uses.

comment:9 Changed 9 years ago by each

  • Summary changed from data source hangs with CNAME loops to review: data source hangs with CNAME loops

comment:10 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:11 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to each

Reviewing revision 2325. Files modified/added since the branch was created at revision 2269:

M src/lib/datasrc/tests/datasrc_unittest.cc
M src/lib/datasrc/data_source.cc
M src/lib/dns/message.h
M src/lib/dns/message.cc

Review of the changes made between 2269 and 2535

src/lib/datasrc/tests/datasrc_unittest.cc
OK

Note: One test is disabled (synthesisedCnameTooLong). When re-enabled, the test fails because the exception TooLongName is thrown. Isn't this expected?

src/lib/datasrc/data_source.cc
OK

Remark: addToMessage() returns no indication is returned as to whether the addition of the RRset to the message succeeded or failed because of an already-existing one. Is there an advantage to returning a bool and having callers test the return value as an internal consistency check?

src/lib/dns/message.h
OK

src/lib/dns/message.cc
OK

Remark: The method addRRset() has a comment "Note: should check duplicate (TBD)". Should the check be done here or (as it now is) in addToMessage()?

comment:12 in reply to: ↑ 11 Changed 9 years ago by each

Replying to stephen:

Note: One test is disabled (synthesisedCnameTooLong). When re-enabled, the test fails because the exception TooLongName is thrown. Isn't this expected?

Exceptions are expensive; we'd prefer not to use them unless we have to. In this case particularly, a caller could force the server to throw an exception just by sending a long qname, which has potential as a DoS vector. It's better to check the qname length before doing the Name concatenation.

Remark: addToMessage() returns no indication is returned as to whether the addition of the RRset to the message succeeded or failed because of an already-existing one. Is there an advantage to returning a bool and having callers test the return value as an internal consistency check?

IMHO, no. The caller doesn't care, it just wants to be know the message got in.

Remark: The method addRRset() has a comment "Note: should check duplicate (TBD)". Should the check be done here or (as it now is) in addToMessage()?

We discussed this in comment:3 and comment:4, above. I saw that comment in Jinmei's code in message.cc, and thought it made sense to do the check there. Jinmei, however, had apparently reconsidered the point and thought it better for the caller to take care of this. After I looked at BIND 9 again I realized that the dns_message module doesn't do duplication checking; the caller does. So I just took the same approach. I'll remove that comment.

comment:13 Changed 9 years ago by each

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.