[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #30344 [Core Tor/Tor]: conn_read_callback is called on connections that are marked for closed



#30344: conn_read_callback is called on connections that are marked for closed
--------------------------+------------------------------
 Reporter:  robgjansen    |          Owner:  (none)
     Type:  defect        |         Status:  new
 Priority:  Medium        |      Milestone:
Component:  Core Tor/Tor  |        Version:  Tor: 0.3.5.8
 Severity:  Normal        |     Resolution:
 Keywords:                |  Actual Points:
Parent ID:                |         Points:
 Reviewer:                |        Sponsor:
--------------------------+------------------------------

Comment (by robgjansen):

 I wonder if the better place to stop reading on marked connections is
 inside of `connection_mark_for_close_internal_`, which appears to be the
 only place (outside of testing code) where the `conn->marked_for_close`
 state variable is written (modified).

 {{{
 diff --git a/src/core/mainloop/connection.c
 b/src/core/mainloop/connection.c
 index f2a646c..e24d349 100644
 --- a/src/core/mainloop/connection.c
 +++ b/src/core/mainloop/connection.c
 @@ -941,6 +941,12 @@ connection_mark_for_close_internal_, (connection_t
 *conn,
     * the number of seconds since last successful write, so
     * we get our whole 15 seconds */
    conn->timestamp_last_write_allowed = time(NULL);
 +
 +  /* We should never listen for read events on marked connections,
 because
 +   * we never try to actually read from the connection again. */
 +  if (connection_is_reading(conn)) {
 +    connection_stop_reading(conn);
 +  }
  }

  /** Find each connection that has hold_open_until_flushed set to
 }}}

 An issue with this approach is even though we disable read events as
 above, we might enable it somewhere else (which would be a bug).

 To ensure that bug never occurs, we could add a check in
 `connection_start_reading` so that we return if the connection is marked.

 {{{
 diff --git a/src/core/mainloop/mainloop.c b/src/core/mainloop/mainloop.c
 index e82c77a..7922156 100644
 --- a/src/core/mainloop/mainloop.c
 +++ b/src/core/mainloop/mainloop.c
 @@ -641,6 +641,10 @@ connection_start_reading,(connection_t *conn))
      return;
    }

 +  if (conn->marked_for_close) {
 +      return;
 +  }
 +
    if (conn->linked) {
      conn->reading_from_linked_conn = 1;
      if (connection_should_read_from_linked_conn(conn))

 }}}

 Or maybe the logic should be added to `connection_check_event`?

 (Note that my diffs are on a custom branch and may not apply cleanly.)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/30344#comment:1>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs