Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5324 closed defect (fixed)

connection_handle_read_impl() has wrong function comment?

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

connection_handle_read_impl() says

 * This function gets called from conn_read() in main.c, either
 * when poll() has declared that conn wants to read, or (for OR conns)
 * when there are pending TLS bytes.

First of all, there is no conn_read() anymore. But also, I suspect this function is no longer called when there are pending TLS bytes?

I worry that libevent only calls the callback when the socket has something to read, and not when the ssl object has pending bytes. That would mean we are forced to keep our "read the whole ssl record even if it means our read bucket goes negative" feature? See #5323.

Child Tickets

Change History (7)

comment:1 Changed 8 years ago by nickm

To answer your questions: connection_handle_read is called from conn_read_callback, when we could read data on the underlying connection. (Ignore the other case.)

Then we do indeed get into the kind of issue you describe. We call SSL_read() on the number of bytes we want (call it N), but OpenSSL can indeed read more than N bytes from the network if it needs to do that in order to get a packet, decrypt it, and read N bytes. Moreover, we don't let the rest of the packet's contents sit in the SSL object's read buffer! Instead, we read it all, here in connection_read_to_buf():

    pending = tor_tls_get_pending_bytes(or_conn->tls);
    if (pending) {
      /* If we have any pending bytes, we read them now.  This *can*
       * take us over our read allotment, but really we shouldn't be
       * believing that SSL bytes are the same as TCP bytes anyway. */
      int r2 = read_to_buf_tls(or_conn->tls, pending, conn->inbuf);
      if (r2<0) {
        log_warn(LD_BUG, "apparently, reading pending bytes can fail.");
        return -1;
      }
    }

That comment appears to be relevant. :)

comment:2 Changed 8 years ago by arma

Yes, I know this is what the code does now. But let's say I wanted to change Tor's behavior so it never "reads" past its read bucket size. (here, "reads" means getting the cells out of openssl's buffers into tor's buffers.) If I just delete all the code you quoted, I get this behavior. But I may have a new bug, which is that Tor never thinks to check if SSL_pending(conn->tls->ssl), because Tor assumed that we would never leave pending bytes pending. The question is: would I in fact have this new bug? Or does libevent consider SSL_pending() and trigger the callback if so?

(If I would have the new bug, is it hard to make libevent trigger the callback when SSL_pending is true, or is this a rat's nest I would rather avoid?)

comment:3 Changed 8 years ago by nickm

Changing that code would indeed create the bug you mention.

Probably the best bet would be, if the connection is bandwidth blocked but there is still pending data, stick it on a linked list of such connections. Later, when its read bucket becomes nonempty, take it off that list and treat it as if it had just become readable.

(If I would have the new bug, is it hard to make libevent trigger the callback when SSL_pending is true, or is this a rat's nest I would rather avoid?)

It's a misuse of Libevent's "event" interface to have them look at something besides the network. It would be a better use of bufferevents, but bufferevents already handle this case. (They handle this in the same way that Tor does, though.) The place to hack would probably be in bufferevent_openssl.c, but that would only affect bufferevent builds.

You could also do this by using filtering bufferevents (which I *think* we've got the bugs out of at this point, mostly-sorta?); in that case, the underlying transport does not read more than its buckets allow it to read.

You could also manually trigger events yourself, but the timing and circumstances for that are weird enough that I think you'd be better off looking at my linked list idea above.

comment:4 Changed 8 years ago by arma

Resolved immediate bug in git commit b5a8c3aa00e. One day we might change Tor to have even more complexity in its read/write process, but that day is not today. Thanks!

comment:5 Changed 8 years ago by arma

Resolution: fixed
Status: newclosed

comment:6 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:7 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.