[tor-bugs] #3302 [Pluggable transport]: obfs2 delays sending pending data

[Tor Bug Tracker & Wiki]

#3302: obfs2 delays sending pending data
---------------------------------+------------------------------------------
 Reporter:  asn                  |          Owner:  asn         
     Type:  defect               |         Status:  needs_review
 Priority:  normal               |      Milestone:              
Component:  Pluggable transport  |        Version:              
 Keywords:                       |         Parent:              
   Points:                       |   Actualpoints:              
---------------------------------+------------------------------------------
Changes (by asn):

  * status:  new => needs_review


Comment:

 Replying to [comment:6 nickm]:
 > Replying to [comment:5 asn]:
 > > Replying to [comment:4 nickm]:
 > > > I really prefer the solution I outlined where obfs2_recv() can
 return something that indicates that the caller needs to do an
 obfs2_send() right away without waiting any data.
 > > >
 > >
 > > Yes, I like that solution myself, but the implementation seems dirtier
 than it sounds on words. Basically, the caller of obfs2_send() is
 proto_send(). proto_send() can do the same things that obfs2_send() can
 do, so we have to go back to plaintext_read_cb(). We can indeed call
 obfs2_send() correctly from there, but isn't it ugly?
 > > I know I'm stuck on the concept of abstraction when we just have one
 protocol, but still defining a protocol specific return code (since
 calling proto_send() right away shouldn't make sense on other protocols)
 on network.c gives me twitches.
 >
 > I like abstraction too: so don't make it a protocol-specific return
 code.  Instead, it could be a generic "You should call proto_send()" thing
 rather than a protocol-specific "You should call obfs2_send()" thing.
 >
 > > > Having the listeners have a list of connections just seems wrong, if
 the whole point is to serve as a socks_state-to-connection map.  If the
 protocol send and recv methods really truly need access to the conn_t,
 then let's just pass them the conn_t!  The functions that call proto_send
 and proto_recv have the conn_t already: no need to make the functions they
 call hunt for it.
 > >
 > > You are right, doing all that just to get a socks_state-to-connection
 map is stupid. I just found it intuitive in the sense that the point of a
 listener is to accept connections and that would give us a way to go from
 a listener to a connection.
 > >
 > > Passing `conn_t` to the protocols is ugly as well, but it provides us
 with ways to solve other similar problems (like #3291).
 >
 > One thing I dislike about the passing conn_t solution is that it makes
 the protocols harder to test: it is much easier to make sure that they do
 the right things with evbuffers than it is to make sure that they do the
 right things with bufferevents.
 >
 > Alternatively, we _could_ just make the protocol state contain a pointer
 to the evbuffer that we want to flush pending data into when the handshake
 completes.  That's a hack too, but at least it isolates everything into
 the protocol layer.

 bug3302 branch fixes this by, indeed, setting a special return code for
 exactly this purpose! And I also think I'm not too sad about this solution
 any more.

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