Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#3135 closed defect (fixed)

Tor segfaults on saveconf if it can't read its torrc file

Reported by: atagar Owned by:
Priority: High Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Hi, in a very weird edge case a controller issuing a saveconf request can terminate Tor. I just reproed this with...
Tor v0.2.3.1-alpha-dev (git-e6980faec43504ac)

Repro steps:

  1. Start tor with an open control port
  2. Chown the torrc to root so the tor user lacks read/write permissions
  3. Issue a SAVECONF request
  4. Tor crashes with...

May 10 18:25:39.000 [warn] Could not open "/home/atagar/.tor/torrc": Permission denied
Segmentation fault

The permissions on the torrc during the crash...
-rw------- 1 root atagar 274 2011-05-10 10:01 .tor/torrc

Here's the quick method for issuing the saveconf request via torctl:

>>> from TorCtl import TorCtl
>>> conn = TorCtl.connect()
>>> conn.sendAndRecv("SAVECONF\r\n")

Cheers! -Damian

Child Tickets

Change History (12)

comment:1 Changed 8 years ago by arma

Milestone: Tor: 0.2.3.x-final
Priority: minormajor
Version: Tor: 0.2.3.1-alpha

Tor isn't supposed to seg fault, ever.

On step 2, does the user need to lack read permissions too?

Can you set your ulimit -c to unlimited and give us the traceback from gdb?

comment:2 Changed 8 years ago by nickm

Does this happen in 0.2.2 as well, or only 0.2.3?

comment:3 in reply to:  1 Changed 8 years ago by atagar

Replying to arma:

On step 2, does the user need to lack read permissions too?

If the user has read permissions to the file then the torrc is successfully replaced (the user has write permissions for the directory in this case).

Can you set your ulimit -c to unlimited and give us the traceback from gdb?

Here's the tracback:

...
May 11 18:52:00.000 [warn] Could not open "/home/atagar/.tor/torrc": Permission denied 

Program received signal SIGSEGV, Segmentation fault.
0x003a123e in strncmp () from /lib/tls/i686/cmov/libc.so.6
(gdb) backtrace
#0  0x003a123e in strncmp () from /lib/tls/i686/cmov/libc.so.6
#1  0x0811fbd5 in strcmpstart (s1=0x0, 
    s2=0x8153fe4 "# This file was generated by Tor; if you edit it, comments will not be preserved") at util.c:504
#2  0x080c36d3 in write_configuration_file () at config.c:4767
#3  options_save_current () at config.c:4842
#4  0x080dfa66 in handle_control_saveconf (conn=0x88957c8) at control.c:1182
#5  connection_control_process_inbuf (conn=0x88957c8) at control.c:3001
#6  0x080c60f5 in connection_process_inbuf (conn=0x0, package_partial=-16843009) at connection.c:3649
#7  0x080ca9a2 in connection_handle_read_impl (conn=0x88957c8) at connection.c:2576
#8  connection_handle_read (conn=0x88957c8) at connection.c:2616
#9  0x08051934 in conn_read_callback (fd=19, event=2, _conn=0x88957c8) at main.c:665
#10 0x0016f248 in event_base_loop () from /usr/lib/libevent-1.4.so.2
#11 0x0804f639 in do_main_loop () at main.c:1784
#12 0x0804f95d in tor_main (argc=3, argv=0xbffff4a4) at main.c:2457
#13 0x0804ddeb in main (argc=3, argv=0xbffff4a4) at tor_main.c:30

comment:4 Changed 8 years ago by arma

Component: Tor RelayTor Client
Milestone: Tor: 0.2.3.x-finalTor: 0.2.1.x-final
Summary: Saveconf request can terminate torTor segfaults on saveconf if it can't read its torrc file
Version: Tor: 0.2.3.1-alpha

Here's the code:

    case FN_FILE:
      old_val = read_file_to_str(fname, 0, NULL);
      if (strcmpstart(old_val, GENERATED_FILE_PREFIX)) {
        rename_old = 1;
      }

We're not checking if read_file_to_str fails. This bug is also in maint-0.2.1.

comment:5 Changed 8 years ago by nickm

Status: newneeds_review

See branch bug3135 in my public repo. This bug has been there since 0.0.9pre6.

comment:6 in reply to:  2 Changed 8 years ago by atagar

Replying to nickm:

Does this happen in 0.2.2 as well, or only 0.2.3?

Just tried it with the Tor v0.2.2.9-alpha (git-2e159967c9871477) and it repros there too.

comment:7 in reply to:  5 Changed 8 years ago by arma

Replying to nickm:

See branch bug3135 in my public repo. This bug has been there since 0.0.9pre6.

Looks plausible.

What's up with the comment in file_status() that says "Return FN_ERROR if filename can't be read"?

comment:8 Changed 8 years ago by nickm

It's bogus; it should be "If the file can't be stat()ed".

Really, though, we shouldn't be doing "if (file_status(x) == FN_FILE) { read_file() }" stuff. There's a potential race condition between when we check the status and when we open it. Instead we should just open the thing and see what happens. That's another bug for another time, though.

comment:9 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Atagar tested this and says it works. Merging and closing. Thanks!

comment:10 Changed 7 years ago by nickm

Keywords: tor-client added

comment:11 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:12 Changed 6 years ago by nickm

Milestone: Tor: 0.2.1.x-final

Milestone Tor: 0.2.1.x-final deleted

Note: See TracTickets for help on using tickets.