Opened 6 years ago

Last modified 21 months ago

#7729 needs_revision defect

Reading pending TLS bytes can take us over at_most

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay tls tls-api mainloop
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Attachments (1)

216239.txt (244 bytes) - added by nickm 6 years ago.

Download all attachments as: .zip

Change History (20)

Changed 6 years ago by nickm

Attachment: 216239.txt added

comment:1 Changed 6 years ago by nickm

Status: newneeds_review

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;
    }
  }

comment:2 Changed 6 years ago by nickm

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) {

comment:3 Changed 6 years ago by nickm

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

comment:4 Changed 6 years ago by cypherpunks

  {
    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;
    }
  }

comment:5 Changed 6 years ago by nickm

yeah, that's even cleaner.

comment:6 Changed 6 years ago by cypherpunks

Resolution: wontfix
Status: needs_reviewclosed

comment:7 Changed 6 years ago by aagbsn

Resolution: wontfix
Status: closedreopened

comment:8 Changed 6 years ago by aagbsn

Status: reopenedneeds_review

comment:9 Changed 6 years ago by andrea

See my bug7729 branch.

comment:10 Changed 6 years ago by cypherpunks

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.

comment:11 Changed 6 years ago by nickm

Keywords: 024-backport added
Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

comment:12 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

comment:13 Changed 4 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???
Status: needs_reviewneeds_revision

comment:14 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:15 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

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

comment:16 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:17 Changed 2 years ago by nickm

Keywords: 024-backport removed

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

comment:18 Changed 2 years ago by nickm

Keywords: tls tls-api mainloop added
Milestone: Tor: unspecifiedTor: 0.3.2.x-final
Severity: Normal

We ought to take another look here.

comment:19 Changed 21 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: unspecified

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

Note: See TracTickets for help on using tickets.