Opened 7 years ago

Closed 7 years ago

#2276 closed defect (fixed)

avoid uninitialized variable from in-memory getClosestNSEC

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

Description

valgrind reported use of some uninitialized variables:
http://git.bind10.isc.org/~tester/builder//BIND10-valgrind/20120920071345-CentOS5-x86_64-GCC/logs/valgrind.out

valgrind seems to be correct. When we do things like:

            const ZoneNode* nsec_node;
            const RdataSet* nsec_rds = getClosestNSEC(zone_data,
                                                      node_path,
                                                      &nsec_node,
                                                      options);

nsec_node can be left uninitialized and subsequently used.

A straightforward fix is to set it to NULL at declaration, but I'd
rather solve it by making the interface safer, i.e., instead of
passing ZoneNode** to getClosestNSEC(), let getClosestNSEC return a
pair of ZoneNode* and RdataSet*.

The attached patch implements the idea.

If we don't immediately apply the change as an urgent care fix, I'd
also like to propose some cleanups:

  • rename find_internal findInternal per naming convention
  • rename some of rrset variables to something like rdset or rdataset. For example, rename the member variable of FindNodeResult:
        const RdataSet* rrset;
    

because naming an RdataSet rrset can be misleading.

  • change the order of the rrset (which should be named rdataset) and node parameters of createFindResult():
    createFindResult(const RRClass& rrclass,
                     const ZoneData& zone_data,
                     ZoneFinder::Result code,
                     const RdataSet* rrset,
                     const ZoneNode* node,
                     bool wild = false,
                     const Name* qname = NULL) {
    
    because it's more consistent with other functions/methods that take both a node and rdataset, and because it would be the more natural order if we consider the pair of them as an RRset.

Subtickets

Attachments (1)

zone_finder.diff (6.2 KB) - added by jinmei 7 years ago.

Download all attachments as: .zip

Change History (8)

Changed 7 years ago by jinmei

comment:1 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 3

comment:2 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20121009

comment:3 Changed 7 years ago by muks

  • Owner set to muks
  • Status changed from new to assigned

Picking

comment:4 Changed 7 years ago by muks

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

Up for review.

comment:5 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:6 Changed 7 years ago by jelte

  • Owner changed from jelte to muks

That looks OK and can be merged.

comment:7 Changed 7 years ago by muks

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

Merged to master in commit 26e55ce110123f7557732ecaeaaad4816cd0375b:

* ae20aca [2276] Change order of arguments to createFindResult()
* 0f05015 [2276] Rename some incorrectly named rrset variables to rdataset
* eff1c6b [2276] Rename find_internal() to findInternal() as per naming convention
* 92daff7 [2276] Add argument that was missing in last commit
* 003b287 [2276] Avoid uninitialized variable from in-memory getClosestNSEC

Resolving as fixed. Thank you for the review Jelte.

Note: See TracTickets for help on using tickets.