#3569 part 1: Rework SOCKS5 server code with trunnel #180
Conversation
squash! Reimplement phase 1 of SOCKS5 using trunnel
We pullup 512 bytes of input to make sure that at least one SOCKS message ends up in head of linked list
Hi! This branch is looking very nice! I have a few small suggestions and requests.
Also, please ignore any comments that you addressed later in your branch. There are a lot of times when I said "this needs documentation" and later you documented it.
Also, please ignore any comments that you addressed later in your branch. There are a lot of times when I said "this needs documentation" and later you documented it.
src/trunnel/socks5.trunnel
Outdated
| u32 addr; | ||
| } | ||
|
|
||
| // And here's the extended stuff from proposal 229 |
Let's remove this stuff: we probably are not going to implement proposal 229.
Removed in 055840a.
src/or/proto_socks.c
Outdated
| @@ -85,6 +86,138 @@ socks_request_free_(socks_request_t *req) | |||
| tor_free(req); | |||
| } | |||
|
|
|||
| static int | |||
There should be a documentation comment for this function (and all new functions).
src/or/proto_socks.c
Outdated
| socks4_client_request_t *trunnel_req; | ||
|
|
||
| ssize_t parsed = | ||
| socks4_client_request_parse(&trunnel_req, (const uint8_t *)raw_data, |
The cast on this line is unnecessary
Fixed (4588211).
src/or/proto_socks.c
Outdated
| goto end; | ||
| } else if (parsed == -2) { | ||
| res = 0; | ||
| if (datalen > 1024) { // XXX |
This should be a named constant, and the warning should probably be different.
No longer comparing datalen to 1024 anywhere in proto_socks.c. Comparing to MAX_SOCKS_MESSAGE_LEN (512 bytes) instead.
src/or/proto_socks.c
Outdated
| if (*is_socks4a) { | ||
| // We cannot rely on trunnel here, as we want to detect if | ||
| // we have abnormally long hostname field. | ||
| char *hostname = (char *)raw_data + SOCKS4_NETWORK_LEN + |
This should use "parsed" instead -- that should tell us the number of bytes that trunnel consumed.
Also, it should be a const char*, since we don't plan to change it.
To detect hostnames that are overly long (one of the testcases is test_socks.c), we cannot generally rely on trunnel, as it will report failure on entire message. That's the reason for doing this workaround here.
Added const in 5785007.
src/or/proto_socks.c
Outdated
| @@ -433,11 +433,134 @@ process_socks5_userpass_auth(socks_request_t *req) | |||
| return res; | |||
| } | |||
|
|
|||
| static int | |||
Needs documentation.
src/or/proto_socks.c
Outdated
| goto end; | ||
| } | ||
|
|
||
| *drain_out = (size_t)parsed; |
Add assertion; initialize to 0.
Fixed.
src/or/proto_socks.c
Outdated
| } | ||
|
|
||
| static int | ||
| process_socks5_client_request(socks_request_t *req, |
Can this argument become const?
Needs docs.
But we may call socks_request_set_socks5_error on this req... Doesn't that violate semantics of const?
src/or/proto_socks.c
Outdated
| @@ -477,8 +600,8 @@ handle_socks_message(const uint8_t *raw_data, size_t datalen, socks_request_t *r | |||
| goto end; | |||
| } | |||
| /* RFC1929 SOCKS5 username/password subnegotiation. */ | |||
| if ((!req->got_auth && raw_data[0] == 1) || | |||
| req->auth_type == SOCKS_USER_PASS) { | |||
| if (!req->got_auth && (raw_data[0] == 1 || | |||
I don't understand the reasoning behind this change?
I don't remember exactly, but it has something to do with how our implicit SOCKS FSM works. If I change it back, 5_authenticate_with_data starts failing.
This code will go away when we rework our SOCKS implementation to function like an explicit state machine.
| case SOCKS_RESULT_DONE: | ||
| res = 1; | ||
| break; | ||
| case SOCKS_RESULT_TRUNCATED: |
This should have a "falls through" comment so GCC doesn't complain.
Fixed in 90f6153.
https://trac.torproject.org/projects/tor/ticket/3569
The text was updated successfully, but these errors were encountered: