[tor-bugs] #7472 [Tor]: Audit all calls to connection_mark_for_close()

[Tor Bug Tracker & Wiki]

#7472: Audit all calls to connection_mark_for_close()
--------------------+-------------------------------------------------------
 Reporter:  andrea  |          Owner:  andrea            
     Type:  task    |         Status:  new               
 Priority:  normal  |      Milestone:  Tor: 0.2.4.x-final
Component:  Tor     |        Version:  Tor: 0.2.4.5-alpha
 Keywords:          |         Parent:                    
   Points:          |   Actualpoints:                    
--------------------+-------------------------------------------------------

Comment(by nickm):

 Replying to [comment:1 andrea]:
 > I've attached my analysis of all occurrences of
 connection_mark_for_close(); the ones potentially meriting a closer look
 are:
 >
 > connection.c:2099:        connection_mark_for_close(conn);
 >  - This is in connection_mark_all_noncontrol_connections(), could be
 >    an erroneous close of an orconn.  What calls this?

 It's used when the DisableNetwork option is turned on while Tor is
 running.

 > connection.c:2804:        connection_mark_for_close(linked);
 >  - Also in connection_handle_read_impl(); tries to flush from linked
 conn
 >    and then connection_flushed_some() fails.  Could possibly be an
 orconn
 >    (when do linked conns get created?); comments suggest this case might
 be
 >    a can't-happen, though.

 Linked connections are currently always an edge (ap or exit) connection
 linked to a directory connection.

 > connection.c:3051:        connection_mark_for_close(conn);
 >  - This is in connection_handle_read_cb(), and gets called if
 >    connection_process_inbuf() fails; could be an orconn?
 >    Comment suggests this might be a can't-happen.

 oops.  That comment is asking, "Should the second argument to
 connection_process_inbuf really always be 1 here"?  It's entirely possible
 for connection_process_inbuf() to return -1 on an orconn; check out
 connection_or_process_inbuf() for examples.

 > connection.c:3063:      connection_mark_for_close(conn);
 >  - This is closely parallel to the above case, but in the
 >    connection_handle_write_cb() function.
 >
 > connection.c:3120:      connection_mark_for_close(conn);
 >  - This is the error-handling case in connection_handle_event_cb(); if
 the
 >    error is during connect and conn is an orconn it will call
 >    connection_or_connect_failed(), but it looks like it probably should
 handle
 >    other orconn cases.

 Yeah; note that the last 3 are bufferevent-specific.

 > connection.c:3270:      connection_mark_for_close(conn);
 >  - Error with getsockopt() in connection_handle_write_impl(); this
 >    should probably call connection_or_notify_error() too if it's an
 orconn.

 agreed

 > connection.c:3391:      connection_mark_for_close(conn);
 >  - I *think* this case can't be an *active* orconn, but it might be
 worth a
 >    test here anyway.  This one can only happen if
 >    !(connection_speaks_cells(conn) && conn->state >
 >        OR_CONN_STATE_PROXY_HANDSHAKING)

 Right.  connection_speaks_cells() is an alias for "is an orconn" right
 now.  So any orconn, other than one doing a HTTP or SOCKS handshake with
 proxy, will hit the first clause of the conditional statement.

 > connection.c:3538:      connection_mark_for_close(conn);
 >  - Error in write_to_buf() or write_to_buf_zlib() from
 >    connection_write_to_buf_impl_(); this should check for orconn, I
 think.

 Yes.  In practice, I don't think these functions can fail, but f they're
 documented to have an error return, we should check for it.

 > main.c:731:      connection_mark_for_close(conn);
 >  - This is conn_read_callback() when connection_handle_read() returns
 >    an error; can this ever happen for an orconn?
 > main.c:769:      connection_mark_for_close(conn);
 >  - This is conn_write_callback() when connection_handle_write() returns
 >    an error; can this ever happen for an orconn?

 These two should never happen for any kind of connection, since they only
 happen if the connection is not already marked, and
 connection_handle_{read,write} are supposed to mark the connection every
 time they return -1.  But that said, there's no reason to think that a bug
 where we forget to mark an orconn is less likely than a bug where we
 forget to mark some other kind of conn.

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