Opened 7 years ago

Closed 6 years ago

#1329 closed defect (fixed)

TorCtl Crashes with Invalid torrc

Reported by: atagar Owned by: mikeperry
Priority: Medium Milestone:
Component: Torctl Version: 0.2.1.24
Severity: Keywords:
Cc: atagar, nickm, arma, mikeperry, Sebastian Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by atagar)

Hi Mike. By issuing a sighup with an invalid torrc (my test case being to set RelayBandwidthBurst <
RelayBandwidthRate) TorCtl dies in a rather unsightly way. Unfortunately I can't prevent this in arm, and if

TorCtl kills the interpreter while curses is running the user's terminal is left in an especially screwed up
state (requiring a reset to function again).

Traceback (most recent call last):
File "/home/atagar/Desktop/arm/TorCtl/TorCtl.py", line 494, in _loop

isEvent, reply = self._read_reply()

File "/home/atagar/Desktop/arm/TorCtl/TorCtl.py", line 636, in _read_reply

line = self._s.readline()

File "/home/atagar/Desktop/arm/TorCtl/TorUtil.py", line 198, in readline

s = self._s.recv(128)

error: (104, 'Connection reset by peer')

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (10)

comment:1 Changed 7 years ago by atagar

Apologies, didn't realize Flyspray would fail to include the top secret, highly advanced feature
known as "line wrapping"... should have known ;)

comment:2 Changed 7 years ago by mikeperry

Can you attach an example of a bad torrc that causes this?

comment:3 Changed 7 years ago by mikeperry

And is ARM the software writing the torrc, or is it "user places new torrc in randomly, decides to HUP tor, and now TorCtl closes the connection"?

comment:4 Changed 7 years ago by atagar

Anything that makes tor complain should do the trick (I already
gave an example, but any invalid syntax should work). Here's the
repro steps:

  • start tor with any valid torrc
  • get a TorCtl connection (in my case I'm using arm)
  • screw up your torrc
  • issue a sighup
  • tor dies, and TorCtl dies with the aforementioned error

arm shouldn't have anything to do with this (though I'm using it
to send the reset signal.

Cheers! -Damian

comment:5 Changed 7 years ago by arma

(At least from Tor's side, Tor is supposed to die when you hup it and the
torrc is invalid. It's better to die than let the user think the hup worked.)

comment:6 Changed 7 years ago by atagar

Yup, realize that. The problem here is that TorCtl's death in that occurrence
is messy, killing the python process using it (and in my case if curses isn't
given the chance to stop nicely it leaves the user's terminal in very bad shape).

comment:7 Changed 7 years ago by nickm

Component: Tor - RelayTor - Torctl

comment:8 Changed 6 years ago by atagar

Description: modified (diff)
Priority: minornormal
Status: newneeds_review

There's two issues here both exhibited from the use case above...

  1. The socket encounters a 104 error after a failed reset. I haven't managed to repro this without arm but it's trivial to fix.
  1. A failed reset causes TorCtl to freeze. This can be repoed with a simple torctl-only use case...
    • start tor with a valid torrc
    • connect torctl
    • screw up the torrc (anything will do)
    • use torctl to send the reset signal (doing a pkill sighup won't exhibit this problem)
>>> import TorCtl
>>> conn = TorCtl.connect()
>>> conn.send_signal("RELOAD")
NOTICE[Mon Jun 13 07:29:41 2011]:Tor closed control connection. Exiting event thread.

At this point TorCtl hangs until the python process is killed. This second issue is because, in _sendImpl line 785:
https://gitweb.torproject.org/pytorctl.git/blob/HEAD:/TorCtl.py#l785

we block until we get a reply which never comes since _loop hit the exception on line 683:
https://gitweb.torproject.org/pytorctl.git/blob/HEAD:/TorCtl.py#l683

I've made a fix for this issue in my 'bug1329' branch which raises the TorCtlClosed to the caller:
https://gitweb.torproject.org/atagar/pytorctl.git/commit/9de33409bb240d8fe50e135dfb895536dc6a45bd

In all the testing I'm throwing at it this seems to work well, however this concurrency handling is pretty confusing so scrutiny welcome. :)

comment:9 Changed 6 years ago by mikeperry

Alright, this one looks good too.

comment:10 Changed 6 years ago by mikeperry

Resolution: Nonefixed
Status: needs_reviewclosed

Merged.

Note: See TracTickets for help on using tickets.