Opened 6 years ago

Closed 6 years ago

#11820 closed defect (fixed)

circuit `NoneType` in obfs3 handshake callbacks

Reported by: asn Owned by: asn
Priority: Medium Milestone:
Component: Archived/Obfsproxy Version:
Severity: Keywords:
Cc: yawning Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Since my obfs3 bridge has gotten more popular, I decided to enable logging and I noticed that I've been getting these errors:

2014-05-06 15:49:51,869 [ERROR] Unhandled error in Deferred:
2014-05-06 15:49:51,870 [ERROR] Unhandled Error
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/Twisted-13.2.0-py2.7-linux-x86_64.egg/twisted/internet/base.py", line 1201, in mainLoop
    self.runUntilCurrent()
  File "/usr/local/lib/python2.7/dist-packages/Twisted-13.2.0-py2.7-linux-x86_64.egg/twisted/internet/base.py", line 797, in runUntilCurrent
    f(*a, **kw)
  File "/usr/local/lib/python2.7/dist-packages/Twisted-13.2.0-py2.7-linux-x86_64.egg/twisted/internet/defer.py", line 382, in callback
    self._startRunCallbacks(result)
  File "/usr/local/lib/python2.7/dist-packages/Twisted-13.2.0-py2.7-linux-x86_64.egg/twisted/internet/defer.py", line 490, in _startRunCallbacks
    self._runCallbacks()
--- <exception caught here> ---
  File "/usr/local/lib/python2.7/dist-packages/Twisted-13.2.0-py2.7-linux-x86_64.egg/twisted/internet/defer.py", line 577, in _runCallbacks
    current.result = callback(current.result, *args, **kw)
  File "/usr/local/lib/python2.7/dist-packages/obfsproxy-0.2.9_1_g4c41868-py2.7.egg/obfsproxy/transports/obfs3.py", line 154, in _uniform_dh_errback
    self.circuit.close()
exceptions.AttributeError: 'NoneType' object has no attribute 'close'

(This used to be #11769 but it now has its own ticket)

Child Tickets

Change History (4)

comment:1 Changed 6 years ago by asn

So, the exception is partially caused by Circuit.close() setting self.transport.circuit to None.
It's also partially caused because the callback/errback of the obfs3 handshake don't check that self.circuit exists.

I think a sequence of events like this would trigger the bug:

(1) User connects. Starts obfs3 handshake.
(2) We start parsing handshake and deferToThread().
(3) User disconnects. Circuit is cleared. `self.transport.circuit` is NULLed.
(4) Our callbacks trigger. They try to access `self.circuit.close()` and they crash.

There are at least a few ways to fix this bug:
a) In the beginning of the callback/errback check that self.circuit exists. If it doesn't, return prematurely since the connection is dead anyway. This will need to become a new rule for transport authors that use threads.
b) Stop setting the transport circuit to None, and guard for self.closed in the various Circuit methods in case the callback/errback try to access them while it's closed.

Both solutions seem acceptable to me.

comment:2 Changed 6 years ago by infinity0

15:24:07 <infinity0> asn: ah guarding close() is fine, i don't think it will hide too much - in this case it wouldn't actually change anything because self.circuit is None so it never reaches circuit.close()

I think we should do both (a) and (b) actually. An extension to (a) would be in the form of documentation to advise "If you write a transport and you want to access self.circut in *an asynchronous callback*, you always have to check if it exists".

A further option in addition to the two above, which would be more code but result in a nicer error message than "NoneType has no attribute close", would be to turn BaseTransport.circuit into a @property, and have a getter that throws an exception ("closed; did you access this inside a callback?") if the underlying variable is None.

comment:3 Changed 6 years ago by asn

I'm not sure if it makes sense checking whether self.circuit exists, if we remove the code that sets it to None.

As a fix to this bug, I'm thinking of using the fix to #11769 (that adds self.closed guards) and also removing the part of the code that sets self.transport.circuit to None.

If this thing bites us again, we can do more elaborate fixes like the @property idea.

comment:4 Changed 6 years ago by asn

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.