Reading pending TLS bytes can take us over at_most

In connection_read_to_buf(), after we fetch pending TLS bytes, we re-set 'result' to be the total number of bytes actually read. But later we do:

  if (more_to_read && result == at_most) {
    slack_in_buf = buf_slack(conn->inbuf);
    at_most = more_to_read;
    goto again;
  }

That's not good; 'result' can also be >= at_most, which might also mean that we should try reading more, maybe.

Reported pseudonymously; the reporter attached this patch. Possibly backportable to 0.2.3.

changed milestone to %Tor: unspecified

added component::core tor/tor mainloop milestone::Tor: unspecified priority::medium severity::normal status::needs-revision tls tls-api tor-relay type::defect labels

Trac:
216239.txt

The patch might be a little more simply written, given that the value of more_to_read is about to get replaced as soon as we do the "goto again".

Perhaps we could just do:

-  if (more_to_read && result == at_most) {
+  if (more_to_read && result >= at_most &&
+      more_to_read > (size_t)(result - at_most)) {
     slack_in_buf = buf_slack(conn->inbuf);
-    at_most = more_to_read;
+    at_most = more_to_read - (result - at_most);
     goto again;
   }

or have the code be:

  {
    ssize_t tmp;
    if (more_to_read && result >= at_most &&
        (tmp = more_to_read - (result - at_most)) > 0) {
      slack_in_buf = buf_slack(conn->inbuf);
      at_most = tmp;
      goto again;
    }
  }

Trac:
Status: new to needs_review

Also, looking at this code, I see that connection_read_to_buf has exactly one caller, which first calls it with -1 in *max_to_read, and then keeps calling with *max_to_read unchanged from however connection_read_to_buf left it.

With this in mind, the part of the code that does:

    *max_to_read = at_most - n_read;

seems like it's probably going to be wrong. Maybe instead we should do something like:

diff --git a/src/or/connection.c b/src/or/connection.c
index 223bbd9..5850e76 100644
--- a/src/or/connection.c
+++ b/src/or/connection.c
@@ -2866,9 +2866,10 @@ connection_read_to_buf(connection_t *conn, ssize_t *max_to_read,
   size_t slack_in_buf, more_to_read;
   size_t n_read = 0, n_written = 0;
 
-  if (at_most == -1) { /* we need to initialize it */
+  if (at_most < 0) { /* we need to initialize it */
     /* how many bytes are we allowed to read? */
     at_most = connection_bucket_read_limit(conn, approx_time());
+    *max_to_readd = at_most;
   }
 
   slack_in_buf = buf_slack(conn->inbuf);
@@ -2979,7 +2980,7 @@ connection_read_to_buf(connection_t *conn, ssize_t *max_to_read,
 
   if (n_read > 0) {
      /* change *max_to_read */
-    *max_to_read = at_most - n_read;
+    *max_to_read -= n_read;
 
     /* Update edge_conn->n_read */
     if (conn->type == CONN_TYPE_AP) {

(That doesn't remove the need for the first fix, of course.)

  {
    if (more_to_read && result >= at_most &&
        (at_most = more_to_read - (result - at_most)) > 0) {
      slack_in_buf = buf_slack(conn->inbuf);
      goto again;
    }
  }

yeah, that's even cleaner.

Trac:
Status: needs_review to closed
Resolution: N/A to wontfix

Trac:
Status: closed to reopened
Resolution: wontfix to N/A

Trac:
Status: reopened to needs_review

See my bug7729 branch.

Such init of max_to_read:

+  if (at_most < 0) { /* we need to initialize it */
     /* how many bytes are we allowed to read? */
     at_most = connection_bucket_read_limit(conn, approx_time());
+    *max_to_read = at_most;
   }

Leads to extra call of connection_read_to_buf() if recv will return EWOULDBLOCK.

It probably better to refactor connection_handle_read_impl() with no loop.

Trac:
Keywords: tor-relay deleted, tor-relay 024-backport added
Milestone: Tor: 0.2.4.x-final to Tor: 0.2.5.x-final

Trac:
Milestone: Tor: 0.2.5.x-final to Tor: 0.2.6.x-final

Trac:
Milestone: Tor: 0.2.6.x-final to Tor: 0.2.???
Status: needs_review to needs_revision

Milestone renamed

Trac:
Milestone: Tor: 0.2.??? to Tor: 0.3.???

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

Trac:
Milestone: Tor: 0.3.??? to Tor: unspecified
Keywords: tor-relay 024-backport deleted, 024-backport, tor-03-unspecified-201612, tor-relay added

Remove an old triaging keyword.

Trac:
Keywords: tor-03-unspecified-201612 deleted, N/A added

None of these is ripe for backport to 0.2.4 even if it does get fixed.

Trac:
Keywords: 024-backport deleted, N/A added

We ought to take another look here.

Trac:
Keywords: tor-relay deleted, tor-relay tls tls-api mainloop added
Sponsor: N/A to N/A
Reviewer: N/A to N/A
Milestone: Tor: unspecified to Tor: 0.3.2.x-final
Severity: N/A to Normal

Mark some needs_revision tickets as unspecified. If/when the revisions happen, they can go back into a live milestone.

Trac:
Milestone: Tor: 0.3.2.x-final to Tor: unspecified

moved to tpo/core/tor#7729 (closed)