Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#3153 closed defect (fixed)

Microdescriptor cache consistency looks error-prone

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

On IRC, wanoskarnet notes that the microdesc_free() function requires that the microdescriptor not be held by any node_t , yet does not check it. He says this is a bug waiting to happen. I agree.

Child Tickets

Change History (6)

comment:1 Changed 8 years ago by nickm

Status: newneeds_review

I'm adding some flags to track microdesc addition and deletion from the nodelist, and also from the microdesc_map. There are two options for how microdesc_free() should handle these flags: It can either assert that everything freed has been removed, or it can remove stuff for us itself, and log that there was a bug.

I'm going with the second approach for now to try to make 0.2.3.x be less crashy. If these warnings show up, though, we will want to track them down.

Please review branch bug3153 in my public repository.

comment:2 Changed 8 years ago by nickm

Wanoskarnet says on IRC that nothing actually calls nodelist_remove_microdesc, and nothing actually clears node->md during normal operation. Better look into that.

comment:3 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

I think wanoskarnet is right here: right now, so long as a microdesc_t is held in a node_t, the microdesc should never actually get freed. That's fine. I'm okay with leaving that function in and unused for now.

Merging the sanity-test code above.

comment:4 Changed 7 years ago by arma

Good thing we chose "fix and warn" rather than assert -- #6429 appears to be such a warning.

comment:5 Changed 6 years ago by nickm

Keywords: tor-client added

comment:6 Changed 6 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.