Refactor fetch_from_buf_socks() to be greedy

Previously, fetch_from_buf_socks() might return 0 if there was still
data on the buffer and a subsequent call to fetch_from_buf_socks()
would return 1.  This was making some of the socks5 unit tests
harder to write, and could potentially have caused misbehavior with
some overly verbose SOCKS implementations.  Now,
fetch_from_buf_socks() does as much processing as it can, and
returns 0 only if it really needs more data.  This brings it into
line with the evbuffer socks implementation.

src/or/buffers.c

@@ -1544,9 +1544,7 @@ fetch_from_buf_socks(buf_t *buf, socks_request_t *req,
     else if (n_drain > 0)
       buf_remove_from_front(buf, n_drain);
 
-  } while (res == 0 && buf->head &&
+  } while (res == 0 && buf->head && want_length < buf->datalen);
-           buf->datalen > buf->head->datalen &&
-           want_length < buf->head->datalen);
 
   return res;
 }
@@ -1690,6 +1688,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req,
       }
       *drain_out = 2u + usernamelen + 1u + passlen;
       req->got_auth = 1;
+      *want_length_out = 7; /* Minimal socks5 sommand. */
       return 0;
     } else if (req->auth_type == SOCKS_USER_PASS) {
       /* unknown version byte */
@@ -1749,8 +1748,8 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req,
       }
       /* we know the method; read in the request */
       log_debug(LD_APP,"socks5: checking request");
-      if (datalen < 8) {/* basic info plus >=2 for addr plus 2 for port */
+      if (datalen < 7) {/* basic info plus >=1 for addr plus 2 for port */
-        *want_length_out = 8;
+        *want_length_out = 7;
         return 0; /* not yet */
       }
       req->command = (unsigned char) *(data+1);
@@ -1891,7 +1890,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req,
           return -1;
         }
         log_debug(LD_APP,"socks4: Username not here yet.");
-        *want_length_out = datalen+1024; /* ???? */
+        *want_length_out = datalen+1024; /* More than we need, but safe */
         return 0;
       }
       authend = next;
@@ -1919,7 +1918,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req,
             return -1;
           }
           log_debug(LD_APP,"socks4: Destaddr not all here yet.");
-          *want_length_out = datalen + 1024;
+          *want_length_out = datalen + 1024; /* More than we need, but safe */
           return 0;
         }
         if (MAX_SOCKS_ADDR_LEN <= next-startaddr) {

src/or/connection_edge.c

@@ -1883,6 +1883,7 @@ connection_ap_handshake_process_socks(edge_connection_t *conn)
   socks_request_t *socks;
   int sockshere;
   or_options_t *options = get_options();
+  int had_reply = 0;
 
   tor_assert(conn);
   tor_assert(conn->_base.type == CONN_TYPE_AP);
@@ -1900,22 +1901,18 @@ connection_ap_handshake_process_socks(edge_connection_t *conn)
     sockshere = fetch_from_buf_socks(conn->_base.inbuf, socks,
                                      options->TestSocks, options->SafeSocks);
   };
+
+  if (socks->replylen) {
+    had_reply = 1;
+    connection_write_to_buf(socks->reply, socks->replylen, TO_CONN(conn));
+    socks->replylen = 0;
+  }
+
   if (sockshere == 0) {
-    if (socks->replylen) {
+    log_debug(LD_APP,"socks handshake not all here yet.");
-      connection_write_to_buf(socks->reply, socks->replylen, TO_CONN(conn));
-      /* zero it out so we can do another round of negotiation */
-      socks->replylen = 0;
-    } else {
-      log_debug(LD_APP,"socks handshake not all here yet.");
-    }
     return 0;
   } else if (sockshere == -1) {
-    if (socks->replylen) { /* we should send reply back */
+    if (!had_reply) {
-      log_debug(LD_APP,"reply is already set for us. Using it.");
-      connection_ap_handshake_socks_reply(conn, socks->reply, socks->replylen,
-                                          END_STREAM_REASON_SOCKSPROTOCOL);
-
-    } else {
       log_warn(LD_APP,"Fetching socks handshake failed. Closing.");
       connection_ap_handshake_socks_reply(conn, NULL, 0,
                                           END_STREAM_REASON_SOCKSPROTOCOL);

src/test/test.c

@@ -381,8 +381,6 @@ test_socks_5_supported_commands(void *ptr)
   /* SOCKS 5 Send CONNECT [01] to FQDN torproject.org:4369 */
   ADD_DATA(buf, "\x05\x01\x00");
   ADD_DATA(buf, "\x05\x01\x00\x03\x0Etorproject.org\x11\x11");
-  test_eq(fetch_from_buf_socks(buf, socks, get_options()->TestSocks,
-                                   get_options()->SafeSocks), 0);
   test_eq(fetch_from_buf_socks(buf, socks, get_options()->TestSocks,
                                    get_options()->SafeSocks), 1);
 
@@ -399,8 +397,6 @@ test_socks_5_supported_commands(void *ptr)
   /* SOCKS 5 Send RESOLVE [F0] request for torproject.org:4369 */
   ADD_DATA(buf, "\x05\x01\x00");
   ADD_DATA(buf, "\x05\xF0\x00\x03\x0Etorproject.org\x01\x02");
-  test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks,
-                                   get_options()->SafeSocks) == 0);
   test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks,
                                    get_options()->SafeSocks) == 1);
   test_eq(5, socks->socks_version);
@@ -415,8 +411,6 @@ test_socks_5_supported_commands(void *ptr)
   /* SOCKS 5 Send RESOLVE_PTR [F1] for IP address 2.2.2.5 */
   ADD_DATA(buf, "\x05\x01\x00");
   ADD_DATA(buf, "\x05\xF1\x00\x01\x02\x02\x02\x05\x01\x03");
-  test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks,
-                                   get_options()->SafeSocks) == 0);
   test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks,
                                    get_options()->SafeSocks) == 1);
   test_eq(5, socks->socks_version);
@@ -528,24 +522,23 @@ test_socks_5_authenticate_with_data(void *ptr)
 
   /* SOCKS 5 Send username/password */
   /* SOCKS 5 Send CONNECT [01] to IP address 2.2.2.2:4369 */
-  ADD_DATA(buf, "\x01\x02me\x02me\x05\x01\x00\x01\x02\x02\x02\x02\x11\x11");
+  ADD_DATA(buf, "\x01\x02me\x03you\x05\x01\x00\x01\x02\x02\x02\x02\x11\x11");
-  test_assert(!fetch_from_buf_socks(buf, socks,
+  test_assert(fetch_from_buf_socks(buf, socks,
                                    get_options()->TestSocks,
-                                   get_options()->SafeSocks));
-  test_eq(5, socks->socks_version);
-  test_eq(2, socks->replylen);
-  test_eq(5, socks->reply[0]);
-  test_eq(0, socks->reply[1]);
-
-  test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks,
                                    get_options()->SafeSocks) == 1);
   test_eq(5, socks->socks_version);
   test_eq(2, socks->replylen);
   test_eq(5, socks->reply[0]);
   test_eq(0, socks->reply[1]);
+
   test_streq("2.2.2.2", socks->address);
   test_eq(4369, socks->port);
 
+  test_eq(2, socks->usernamelen);
+  test_eq(3, socks->passwordlen);
+  test_memeq("me", socks->username, 2);
+  test_memeq("you", socks->password, 3);
+
  done:
   ;
 }