commit 05c424f4b830e98595b33397b58504462dbda8db Author: Nick Mathewson <nickm@torproject.org> Date: Wed Jun 29 17:44:29 2011 -0400 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 | 13 ++++++------- src/or/connection_edge.c | 23 ++++++++++------------- src/test/test.c | 23 ++++++++--------------- 3 files changed, 24 insertions(+), 35 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 44a492a..1310b42 100644 --- a/src/or/buffers.c +++ b/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 && - buf->datalen > buf->head->datalen && - want_length < buf->head->datalen); + } while (res == 0 && buf->head && want_length < buf->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 */ - *want_length_out = 8; + if (datalen < 7) {/* basic info plus >=1 for addr plus 2 for port */ + *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) { diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 4e988bd..bff73d4 100644 --- a/src/or/connection_edge.c +++ b/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) { - 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."); - } + log_debug(LD_APP,"socks handshake not all here yet."); return 0; } else if (sockshere == -1) { - if (socks->replylen) { /* we should send reply back */ - 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 { + if (!had_reply) { log_warn(LD_APP,"Fetching socks handshake failed. Closing."); connection_ap_handshake_socks_reply(conn, NULL, 0, END_STREAM_REASON_SOCKSPROTOCOL); diff --git a/src/test/test.c b/src/test/test.c index 64916ea..01fb30c 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -382,8 +382,6 @@ test_socks_5_supported_commands(void *ptr) 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); test_eq(5, socks->socks_version); @@ -400,8 +398,6 @@ test_socks_5_supported_commands(void *ptr) 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); test_eq(2, socks->replylen); @@ -416,8 +412,6 @@ test_socks_5_supported_commands(void *ptr) 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); test_eq(2, socks->replylen); @@ -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"); - test_assert(!fetch_from_buf_socks(buf, socks, + ADD_DATA(buf, "\x01\x02me\x03you\x05\x01\x00\x01\x02\x02\x02\x02\x11\x11"); + 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: ; }