[tor-bugs] #27750 [Core Tor/Tor]: conn_close_if_marked: Non-fatal assertion !(connection_is_writing(conn))

[Tor Bug Tracker & Wiki]

#27750: conn_close_if_marked: Non-fatal assertion !(connection_is_writing(conn))
-------------------------------------------------+-------------------------
 Reporter:  dgoulet                              |          Owner:  dgoulet
     Type:  defect                               |         Status:  new
 Priority:  Very High                            |      Milestone:  Tor:
                                                 |  0.3.5.x-final
Component:  Core Tor/Tor                         |        Version:  Tor:
                                                 |  0.3.4.8
 Severity:  Normal                               |     Resolution:
 Keywords:  regression, assert, 035-must, 035    |  Actual Points:
  -rc-blocker?                                   |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by dgoulet):

 Ok, here is my analysis (we are in mainloop.c, function
 `conn_close_if_marked()`:

 The idea of the function is to do some actions for a connection that is
 marked "for close". Tor must flush everything on the wire for a connection
 that is about to get closed thus it looks at the *write* bucket and see if
 it can:

 `ssize_t sz = connection_bucket_write_limit(conn, now);`

 Then it tries to flush regardless of the value in `sz` which if `sz = 0`
 then `retval` will be `0` as well. Which brings us to where the `BUG()`
 happens in the condition:

 {{{
       } else if (sz == 0) {
         /* Also, retval==0.  If we get here, we didn't want to write
 anything
          * (because of rate-limiting) and we didn't. */
 }}}

 The whole point of this condition iiuc is to keep the connection "alive"
 because it is being rate limited but still has data to flush (we check
 `connection_wants_to_flush()`).

 I believe #28089 fixed this for the `is_writing()` case and now we are
 only seeing the `is_reading()` case on tor >= 0.3.4.8.

 `connection_consider_empty_read_buckets(conn);` can most of the time *not*
 stop the connection from reading because simply the bucket aren't empty
 for the read() part of the connection. Which means that we will hit the
 `BUG()` probably often when the connection can't write anymore. From the
 code:

 {{{
         if (BUG(connection_is_reading(conn))) {
           /* XXXX+ We should make this code unreachable; if a connection
 is
            * marked for close and flushing, there is no point in reading
 to it
            * at all. Further, checking at this point is a bit of a hack:
 it
            * would make much more sense to react in
            * connection_handle_read_impl, or to just stop reading in
            * mark_and_flush */
           connection_read_bw_exhausted(conn, true/* kludge. */);
         }
 }}}

 I agree with the comment. We shouldn't even `BUG()` there in the first
 place, actually, it makes no sense to call the bw exhausted. We should
 simply stop reading() because the connection is marked for close and move
 on regardless of the read state.

 Again, this condition is only relevant if we _need_ to flush data on the
 wire but we are prevented to do so because of rate limit.

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