[tor-bugs] #7164 [Tor]: microdesc.c:378: Bug: microdesc_free() called, but md was still referenced 1 node(s); held_by_nodes == 1

[Tor Bug Tracker & Wiki]

#7164: microdesc.c:378: Bug: microdesc_free() called, but md was still referenced
1 node(s); held_by_nodes == 1
------------------------+-------------------------------------
     Reporter:  jaj123  |      Owner:
         Type:  defect  |     Status:  needs_review
     Priority:  major   |  Milestone:  Tor: 0.2.5.x-final
    Component:  Tor     |    Version:  Tor: 0.2.4.19
   Resolution:          |   Keywords:  tor-client 024-backport
Actual Points:          |  Parent ID:
       Points:          |
------------------------+-------------------------------------

Comment (by andrea):

 Replying to [comment:30 nickm]:
 > The line number suggests that this is happening in
 microdesc_cache_clean():
 >
 > {{{
 >   for (mdp = HT_START(microdesc_map, &cache->map); mdp != NULL; ) {
 >     if ((*mdp)->last_listed < cutoff) {
 >       ++dropped;
 >       victim = *mdp;
 >       mdp = HT_NEXT_RMV(microdesc_map, &cache->map, mdp);
 >       victim->held_in_map = 0;
 >       bytes_dropped += victim->bodylen;
 >       microdesc_free(victim);
 >     } else {
 >       ++kept;
 >       mdp = HT_NEXT(microdesc_map, &cache->map, mdp);
 >     }
 >   }
 > }}}
 >
 > So, there's a microdesc that is (probably) held by a node, but its last-
 listed is more than one week ago.  Interesting!
 >
 > In theory:
 >   * A node should not exist unless it has a routerstatus or a
 routerinfo.
 >   * A node should not have a microdescriptor unless it has a
 routerstatus.
 >   * Whenever a networkstatus is loaded, we should be updating the
 last_seen field of the microdescriptors.
 >
 > So something has gone wrong with the theory.
 >
 > I'm not too sure what -- if somebody has ideas, that would be great.
 I've tried to write an improved diagnostic branch.  Please review
 "bug7164_diagnose_harder" in my public repository.  It's more logs, not a
 fix.

 Doesn't this also change the behavior as well?

 {{{
 -    if ((*mdp)->last_listed < cutoff) {
 +    const int is_old = (*mdp)->last_listed < cutoff;
 +    const unsigned held_by_nodes = (*mdp)->held_by_nodes;
 +    if (is_old && !held_by_nodes) {
 }}}

 The test that would match the old behavior would be just if (is_old),
 surely?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/7164#comment:33>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online