Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#847 closed defect (fixed)

STLPort regression in b10-resolver

Reported by: jinmei Owned by: vorner
Priority: high Milestone: Sprint-20110503
Component: resolver Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 1.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The debug mode of STLPort reported the following bugs in src/binresolver/tests:

[ RUN      ] ResponseScrubberTest.ScrubAllSectionsValid
/export/home/opt/sunstudio12.1/prod/include/CC/stlport4/stl/debug/_iterator.h(157): STL assertion failure : _Incrementable(*this,1,_Iterator_category())

[ RUN      ] ResponseScrubberTest.ScrubAllSectionsInvalid
/export/home/opt/sunstudio12.1/prod/include/CC/stlport4/stl/debug/_iterator.h(157): STL assertion failure : _Incrementable(*this,1,_Iterator_category())

[ RUN      ] ResponseScrubberTest.CrossSectionAnswer
/export/home/opt/sunstudio12.1/prod/include/CC/stlport4/stl/debug/_iterator.h(157): STL assertion failure : _Incrementable(*this,1,_Iterator_category())

[ RUN      ] ResponseScrubberTest.All
/export/home/opt/sunstudio12.1/prod/include/CC/stlport4/stl/debug/_iterator.h(157): STL assertion failure : _Incrementable(*this,1,_Iterator_category())

Subtickets

Change History (8)

comment:1 Changed 9 years ago by stephen

  • Milestone changed from Year 3 Task Backlog to Sprint-20110503

comment:2 Changed 9 years ago by vorner

  • Defect Severity set to N/A
  • Owner set to vorner
  • Status changed from new to accepted
  • Sub-Project set to DNS

comment:3 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

It should be fixed. The problem was, when the iterator got invalidated by the remove, a variable was set so another iteration of the loop was not entered, but the iterator was increased once before checking the condition. It should be harmless in real life, since the vector iterator is only a pointer, so increasing it and not using doesn't matter, but I fixed it when I found it.

The branch contains first commit of #846, for the same reason.

comment:4 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

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

Replying to vorner:

It should be fixed. The problem was, when the iterator got invalidated by the remove, a variable was set so another iteration of the loop was not entered, but the iterator was increased once before checking the condition. It should be harmless in real life, since the vector iterator is only a pointer, so increasing it and not using doesn't matter, but I fixed it when I found it.

The patch seems to correctly fix the reported problem, and I confirmed
that on the Solaris box. So please feel free to merge it.

A supplemental comment not directly related to the patch: I'd reverse
the semantics and name of variable 'nomatch', i.e., name it 'match'
with the default value of false, set it to 'true' when the if
condition in the inner most for loop is met, and remove the entry if
!match. I'd leave it to you whether to make this change when you
merge it.

I have even a higher level comment (again, not specific to this
patch). IMO this method is too complicated and tricky to understand.
It's difficult to follow how the triple-nested loops work, with
trucking subtle implication of various non constant variables, kept,
(no)match, removed. If we make the RRsetIterators more complete, I
believe we can simplify the whole logic and the resulting code will be
more understandably. For example, if we add something like
vector::erase() to Message (or revised Message::removeRRset() that
way), we'd be able to update the entire code like this:

    message.eraseSection(remove_if(message.beginSection(section),
                                   message.endSection(section),
                                   NameMatch(names)),
                         message.endSection(section));

(where NameMatch? class has operator() that implements the match logic
in the innermost for loop of the original code).

I understand this is far beyond the scope of this ticket, so I'd
propose creating a separate ticket for this.

comment:6 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:7 Changed 9 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thanks. I created ticket for the simplification and merged.

comment:8 Changed 9 years ago by vorner

  • Estimated Difficulty changed from 0.0 to 1
Note: See TracTickets for help on using tickets.