Opened 9 years ago

Closed 6 years ago

#254 closed defect (duplicate)

we should revisit RRset::setName()

Reported by: jinmei Owned by: UnAssigned
Priority: medium Milestone: Sprint-20131015
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket:
Total Hours: Internal?: no

Description

While working on trac #249, I noticed the top level data source could override (the content of) RRsetPtr in wildcard handling:

            RRsetPtr rrset = wild.findRRset(RRType::CNAME(), q.qclass());
            if (rrset != NULL) {
                rrset->setName(task->qname);
                m.addRRset(Section::ANSWER(), rrset, q.wantDnssec());
                chaseCname(q, task, rrset);
            }
...
            BOOST_FOREACH (RRsetPtr rrset, wild) {
                rrset->setName(task->qname);
                m.addRRset(Section::ANSWER(), rrset, q.wantDnssec());
            }

I now think this is a bad practice, because the underlying data source may want to hold the original RRset intact (in this case, with the wildcard owner name). The current style of implementation will also affect hot spot caching because the cached wildcard RRset will be overridden, and will never match a wildcard request (this does not necessarily cause an incorrect behavior, but leads to suboptimial performance).

IMO, what we should do is to make a copy of the returned RRset with a different name. Further, we should generally change the data source interface so that the returned RRsetPtrs are "const" (I once tried that, but it was not very easy due to other dependency). Then, as far as I can see this is the only usage of setName() method, so we can just remove it from the class.

I'm putting this on the backlog.

Subtickets

Change History (12)

comment:1 Changed 9 years ago by jinmei

  • billable set to 0
  • Internal? unset
  • Owner changed from jinmei to UnAssigned
  • Status changed from new to assigned

comment:2 Changed 9 years ago by stephen

  • Milestone feature backlog item deleted

Milestone feature backlog item deleted

comment:3 follow-up: Changed 8 years ago by shane

  • Defect Severity set to N/A
  • Owner changed from UnAssigned to jinmei
  • Sub-Project set to DNS

This code is not untouched, but seems to still have the same basic issue, as far as I understand it.

The code in data_source.cc now reads:

        if (cname) {
            RRsetPtr rrset = findRRsetFromList(wild, RRType::CNAME());
            if (rrset != NULL) {
                rrset->setName(task->qname);
                addToMessage(q, Message::SECTION_ANSWER, rrset);
                chaseCname(q, task, rrset);
            }
        } else {
            BOOST_FOREACH (RRsetPtr rrset, wild) {
                rrset->setName(task->qname);
                addToMessage(q, Message::SECTION_ANSWER, rrset);
            }

Jinmei, can you confirm that this still should be addressed?

Is this something we need to change the constness of the findRRsetFromList() return value? Or perhaps update the documentation of that method?

Has this been discussed on bind10-dev?

comment:4 Changed 8 years ago by shane

  • Defect Severity changed from N/A to Low

comment:5 in reply to: ↑ 3 Changed 8 years ago by jinmei

  • Milestone set to New Tasks

Replying to shane:

This code is not untouched, but seems to still have the same basic issue, as far as I understand it.

The code in data_source.cc now reads:

        if (cname) {
            RRsetPtr rrset = findRRsetFromList(wild, RRType::CNAME());
            if (rrset != NULL) {
                rrset->setName(task->qname);
                addToMessage(q, Message::SECTION_ANSWER, rrset);
                chaseCname(q, task, rrset);
            }
        } else {
            BOOST_FOREACH (RRsetPtr rrset, wild) {
                rrset->setName(task->qname);
                addToMessage(q, Message::SECTION_ANSWER, rrset);
            }

Jinmei, can you confirm that this still should be addressed?

It should still be addressed, and, once again, will be resolved when
we migrate to the new data source implementation/API. I'd keep this
ticket so that we won't forget the cleanup in the libdns++ side.

comment:6 Changed 8 years ago by jinmei

  • Owner changed from jinmei to shane

comment:7 Changed 8 years ago by shane

  • Milestone New Tasks deleted
  • Owner changed from shane to UnAssigned

comment:8 Changed 7 years ago by jinmei

I believe we can deprecate (remove) the setName method
now that we've switched to the new datasrc API.

I'm not sure if it's a good topic for the hardening, but I'm pushing
it to the queue just in case.

Would depend on #2161.

Last edited 7 years ago by jinmei (previous) (diff)

comment:9 Changed 7 years ago by jinmei

  • Defect Severity changed from Low to Medium
  • Milestone set to Next-Sprint-Proposed

comment:10 Changed 6 years ago by shane

  • Milestone set to Sprint-20131015
  • Summary changed from we should revisit RRset::setName() to [kean] we should revisit RRset::setName()

comment:11 Changed 6 years ago by muks

Please close #2335 when this is done.

comment:12 Changed 6 years ago by muks

  • Resolution set to duplicate
  • Status changed from assigned to closed
  • Summary changed from [kean] we should revisit RRset::setName() to we should revisit RRset::setName()

Please don't pick this ticket. I've fixed #2335 and will resolve this ticket as part of it.

Note: See TracTickets for help on using tickets.