Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4340 closed defect (fixed)

0.2.2 relays crash without geoip files

Reported by: nickm Owned by:
Priority: Very High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Found via chutney:

#0  0x00078878 in geoip_note_client_seen (action=GEOIP_CLIENT_NETWORKSTATUS, addr=2130706433, now=1319812719) at geoip.c:439
#1  0x00057cd6 in directory_handle_command (conn=0x21a3d0) at directory.c:2703
#2  0x000590e7 in connection_dir_process_inbuf (conn=0x21a3d0) at directory.c:2162
#3  0x0002deb4 in connection_process_inbuf (conn=0x21a3d0, package_partial=<value temporarily unavailable, due to optimizations>) at connection.c:3363
#4  0x000328ae in connection_handle_read (conn=0x21a3d0) at connection.c:2524
#5  0x000806bf in conn_read_callback (fd=15, event=2, _conn=0x21a3d0) at main.c:514
warning: .o file "/Users/nickm/src/libevent-1.4/.libs/event.o" more recent than executable timestamp
#6  0x001a8911 in event_base_loop (base=0x806400, flags=0) at event.c:401
#7  0x0007df52 in do_main_loop () at main.c:1566
#8  0x0007f73b in tor_main (argc=0, argv=0x0) at main.c:2228
#9  0x00002426 in start ()

For some reason, I haven't seen this in master yet.

What shall we do? The easiest option is to make geoip_note_client_seen() return early if geoip_countries is NULL. But perhaps we should go over the whole file to look for other places where we might be saying "smartlist_len(geoip_countries)" without first making sure geoip_countries is set. And perhaps also we should disallow being an authority without a geoip file.

Child Tickets

Change History (16)

comment:1 Changed 8 years ago by Sebastian

Being a dirauth without a geoip file is handy for privnets, but not required at all if that makes our lives easier.

comment:2 Changed 8 years ago by karsten

Interesting. I'd think a being a dirauth shouldn't depend on having a geoip file.

I can look into this issue or review a patch some time in the next few days.

comment:3 Changed 8 years ago by Sebastian

Status: newneeds_review

This doesn't only crash dirauths, it crashes everyone without a geoip db file. Patch in bug4340 in my repo. Needs changes file and stuff, will add that as fixup

comment:4 Changed 8 years ago by Sebastian

Priority: normalcritical

(And yeah, I looked at all other stats related code in geoip.c and I think I found all the hairy stuff. I added two more safeguards to the _term functions, because calling those would also cause a crash (we currently don't call them that way, but better safe than sorry))

comment:5 Changed 8 years ago by Sebastian

Alternatively, we backport da9190013510412cec00e38c064c5c718d032f9e. It's short and sweet and fixes the issue completely, but doesn't add safeguards.

comment:6 Changed 8 years ago by Sebastian

That also explains why master is unaffected. We merged my patch only into master, not into maint-0.2.2. Dumb me :[

comment:7 Changed 8 years ago by karsten

Replying to Sebastian:

Alternatively, we backport da9190013510412cec00e38c064c5c718d032f9e. It's short and sweet and fixes the issue completely, but doesn't add safeguards.

Backporting da919001 is my preference. No need to solve the same problem in two different ways.

Sorry for not seeing this before. I tested that a 0.2.2.x relay with my patch a) doesn't crash immediately without a geoip file and b) successfully writes stats with a geoip file in the default config. What I didn't test was c) that it doesn't break without a geoip file after some time. Bah.

comment:8 Changed 8 years ago by karsten

Component: Tor Directory AuthorityTor Relay
Summary: 0.2.2 Authorities crash without geoip files0.2.2 relays crash without geoip files

Updating the summary and component.

comment:9 Changed 8 years ago by Sebastian

Oh, not dumb me. We backported the enable stats thing. But yeah, we should probably just do the backport and move on

comment:10 Changed 8 years ago by Sebastian

See branch bug4340_backported in my repo for the backport

comment:11 Changed 8 years ago by Sebastian

rfc2460 from #tor tested the backport patch and can't get his tor to crash anymore with it. I'm currently running some tests myself

comment:12 Changed 8 years ago by Sebastian

Backport has also been tested by me and karsten (i set up two relays, one with the backport, one with the original code. Karsten fetched consensus from both, one died, the other lives)

comment:13 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merging; thanks!

comment:14 Changed 8 years ago by nickm

I think we should open a new ticket for the lower-level fixes, though. To me, the current approach seems error-prone: it makes a set of functions that we must not call if we don't have geoip data.

comment:15 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:16 Changed 7 years ago by nickm

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