[tor-bugs] #5053 [Tor]: Fix IPv6 implementation for bridge statistics

[Tor Bug Tracker & Wiki]

#5053: Fix IPv6 implementation for bridge statistics
-----------------------------+----------------------------------------------
 Reporter:  karsten          |          Owner:  ln5               
     Type:  enhancement      |         Status:  needs_review      
 Priority:  major            |      Milestone:  Tor: 0.2.3.x-final
Component:  Tor              |        Version:                    
 Keywords:  ipv6 tor-bridge  |         Parent:                    
   Points:                   |   Actualpoints:                    
-----------------------------+----------------------------------------------
Changes (by ln5):

  * status:  needs_revision => needs_review


Comment:

 Replying to [comment:33 nickm]:
 > okay, I'll try to review the whole branch here...
 >
 > Stuff we could fix after merge:
 Or before I guess. Please see branch bug5053-bug5055 in my public repo.

 >  * The duplicated code in the new geoip*ipv6() functions is scary. Gotta
 refactor that out.  Cut-and-pasting in programming is begging for bloat
 and bugs.
 Agreed. Commit 6a241ff3 merges geoip_ipv*_add_entry() and
 geoip_ipv*_parse_entry().

 >  * Passing in6_addr by value? it is a 16-byte structure; that kind of
 thing one usually passes by reference.
 Gone with 6a241ff3.

 >  * Comparing in6_addrs with memcmp is iffy; there's no guarantee that
 they can't contain padding or uninitialized fields. If we're going to use
 memcmp, we should probably be comparing  the s8_addr fields.
 Fixed in commit e7e68b80.
 Or do you prefer comparing the s6_addr32's without memcmp?

 >  * The comment that says that the in6_addrs are in "host order" seems
 wrong.
 Gone with e7e68b80.

 >  * The comment /* No alignment issue here, since _key really is a
 pointer to uint32_t */ seems wrong.
 Gone with e7e68b80.

 >  * The clear_geoip_db(); that got removed from geoip_load_file() : where
 did it go?  It looks like we just removed it entirely, which means that
 reloading a geoip file will wind up with entries both from the old file
 and the new one.  That's a big bug.
 geoip_load_file() frees geoip_ipv*_entries before adding new data.
 geoip_ipv*_add_entry() only adds to geoip_countries data not already
 present.

 >  * Gotta fix all the XXX5053; are there any left?
 No.

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