BIND 10 #80: review: auth server incorrectly returns SERVFAIL to queries of class ANY

[BIND 10 Development]

#80: review: auth server incorrectly returns SERVFAIL to queries of class ANY
-------------------------------------+-------------------------------------
                 Reporter:  jinmei   |                Owner:  jinmei
                     Type:  defect   |               Status:  reviewing
                 Priority:           |            Milestone:  A-Team-
  critical                           |  Sprint-20110316
                Component:           |           Resolution:
  b10-auth                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:
Estimated Number of Hours:  5.0      |          Total Hours:
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:21 vorner]:

 > It seems g++ doesn't like passing a function-local class as a template
 argument, I'm not sure if this is OK by the standard or it is a bug. But
 I've taken the class outside, since it's a .cc file anyway, so it won't
 spam much of a namespace.

 I don't know about what the standard says either, but in any event I'm
 okay with the change.

 > Secondly, there are things like this:
 {{{
 RRsetPtr cname_rrset = findRRsetFromList(syn,
                                          RRType::CNAME());
 addToMessage(q, Message::SECTION_ANSWER, cname_rrset);
 chaseCname(q, task, cname_rrset);
 }}}
 >
 > As I get it, findRRsetFromList can return null pointer. Are the other
 functions expected to handle it? Or, is it so it can't happen in these
 cases that the type would be missing?

 I'd first like to note that this patch shouldn't change the behavior
 regarding the possible null pointer case, so the question is not
 specific to this patch but should apply to the original code, too.
 On top of that, my general understanding is that when the code omits
 null pointer check it means the pointer actually cannot be null in
 that context.

 In the above specific example, synthesizeCname() should ensure "syn"
 contains a CNAME RR(set) that makes the call to findRRsetFromList()
 successful.  The only possible exception is the case where the
 underlying data source implementation is buggy and returns an empty
 DNAME RRset, but that case is rejected by the check for sys.size()
 (see notes below though).

 We could still add an explicit check and throw an exception so that if
 the code overlooks minor exceptional cases that breaks the assumption
 then the program will still not crash but return a SERVFAIL.  But I
 would personally have it crashed in that case rather than obscuring
 the bug.

 Note: I admit the code around this logic isn't clean and is difficult
 to follow (but I note it was not written by me).  We should rather
 make synthesizeCname never fails (or has it throw for buggy data
 sources) so that we can omit the syn.size() check, and we should even
 get rid of the use of RRsetList in this case because we actually don't
 expect a list of (multiple types of) RRsets in this context, etc.  At
 the risk of deferring so many cleanups for the data source
 implementation, I'm hoping we can make it much cleaner in our
 scheduled refactoring/redesigning the data source API and implementation.

-- 
Ticket URL: <https://bind10.isc.org/ticket/80#comment:22>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development