Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3302 closed defect (fixed)

obfs2 delays sending pending data

Reported by: asn Owned by: asn
Priority: Medium Milestone:
Component: Archived/Obfsproxy Version:
Severity: Keywords:
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

https://trac.torproject.org/projects/tor/ticket/3202#comment:7

My current fix is not really nice, since it sends up the queued 
data only on the next proto_send(), but I didn't see a way of 
getting the correct evbuffer from inside obfs2_recv().

The problem with fixing this bug (and probably #3291) properly is that there are not many network/libevent stuff exposed to obfs2.
This is of course good, but in the case of these two bugs it forces us do ugly solutions like the above.

For example, in this case if I had access to the conn_t struct, I could use the input/output bufferevents to correctly send the pending data.
In the case of #3291, I could use evtimers with the correct conn_t struct to schedule a conn_free() in the future.

To implement the above, I started creating a linked list in every listener_t that contained all the current connections on it. This is both intuitive and it would allow me to create some 'ugly but
effective, and probably cleaner than other solutions' functions like get_conn_from_socks_state(socks_state_t *socks).

Any feedback on this, nickm? (and others)

Child Tickets

Change History (11)

comment:1 Changed 8 years ago by asn

Component: - Select a componentPluggable transport

comment:2 in reply to:  description ; Changed 8 years ago by rransom

Replying to asn:

https://trac.torproject.org/projects/tor/ticket/3202#comment:7

My current fix is not really nice, since it sends up the queued 
data only on the next proto_send(), but I didn't see a way of 
getting the correct evbuffer from inside obfs2_recv().

The problem with fixing this bug (and probably #3291) properly is that there are not many network/libevent stuff exposed to obfs2.
This is of course good, but in the case of these two bugs it forces us do ugly solutions like the above.

For example, in this case if I had access to the conn_t struct, I could use the input/output bufferevents to correctly send the pending data.

Then specify the conn_t * as the ‘user data’ for the libevent callbacks you're using.

In the case of #3291, I could use evtimers with the correct conn_t struct to schedule a conn_free() in the future.

To implement the above, I started creating a linked list in every listener_t that contained all the current connections on it. This is both intuitive and it would allow me to create some 'ugly but effective, and probably cleaner than other solutions' functions like get_conn_from_socks_state(socks_state_t *socks).

What if two connections are in the same SOCKS state?

comment:3 in reply to:  2 Changed 8 years ago by asn

Hi rransom! Thanks for the interest.

Replying to rransom:

Replying to asn:

https://trac.torproject.org/projects/tor/ticket/3202#comment:7

My current fix is not really nice, since it sends up the queued 
data only on the next proto_send(), but I didn't see a way of 
getting the correct evbuffer from inside obfs2_recv().

The problem with fixing this bug (and probably #3291) properly is that there are not many network/libevent stuff exposed to obfs2.
This is of course good, but in the case of these two bugs it forces us do ugly solutions like the above.

For example, in this case if I had access to the conn_t struct, I could use the input/output bufferevents to correctly send the pending data.

Then specify the conn_t * as the ‘user data’ for the libevent callbacks you're using.

Passing conn_t * to proto_recv() just for the case that we have pending data (which generally shouldn't happen) *on the obfs2 protocol* is quite ugly as well, isn't it?

In the case of #3291, I could use evtimers with the correct conn_t struct to schedule a conn_free() in the future.

To implement the above, I started creating a linked list in every listener_t that contained all the current connections on it. This is both intuitive and it would allow me to create some 'ugly but effective, and probably cleaner than other solutions' functions like get_conn_from_socks_state(socks_state_t *socks).

What if two connections are in the same SOCKS state?

SOCKS state is created per-connection basis and is attached to the conn_t * like this:

conn->socks_state = socks_state_new();

comment:4 Changed 8 years ago by nickm

I really prefer the solution I outlined where obfs2_recv() can return something that indicates that the caller needs to do an obfs2_send() right away without waiting any data.

Having the listeners have a list of connections just seems wrong, if the whole point is to serve as a socks_state-to-connection map. If the protocol send and recv methods really truly need access to the conn_t, then let's just pass them the conn_t! The functions that call proto_send and proto_recv have the conn_t already: no need to make the functions they call hunt for it.

comment:5 in reply to:  4 ; Changed 8 years ago by asn

Replying to nickm:

I really prefer the solution I outlined where obfs2_recv() can return something that indicates that the caller needs to do an obfs2_send() right away without waiting any data.

Yes, I like that solution myself, but the implementation seems dirtier than it sounds on words. Basically, the caller of obfs2_send() is proto_send(). proto_send() can do the same things that obfs2_send() can do, so we have to go back to plaintext_read_cb(). We can indeed call obfs2_send() correctly from there, but isn't it ugly?
I know I'm stuck on the concept of abstraction when we just have one protocol, but still defining a protocol specific return code (since calling proto_send() right away shouldn't make sense on other protocols) on network.c gives me twitches.

Having the listeners have a list of connections just seems wrong, if the whole point is to serve as a socks_state-to-connection map. If the protocol send and recv methods really truly need access to the conn_t, then let's just pass them the conn_t! The functions that call proto_send and proto_recv have the conn_t already: no need to make the functions they call hunt for it.

You are right, doing all that just to get a socks_state-to-connection map is stupid. I just found it intuitive in the sense that the point of a listener is to accept connections and that would give us a way to go from a listener to a connection.

Passing conn_t to the protocols is ugly as well, but it provides us with ways to solve other similar problems (like #3291).

comment:6 in reply to:  5 ; Changed 8 years ago by nickm

Replying to asn:

Replying to nickm:

I really prefer the solution I outlined where obfs2_recv() can return something that indicates that the caller needs to do an obfs2_send() right away without waiting any data.

Yes, I like that solution myself, but the implementation seems dirtier than it sounds on words. Basically, the caller of obfs2_send() is proto_send(). proto_send() can do the same things that obfs2_send() can do, so we have to go back to plaintext_read_cb(). We can indeed call obfs2_send() correctly from there, but isn't it ugly?
I know I'm stuck on the concept of abstraction when we just have one protocol, but still defining a protocol specific return code (since calling proto_send() right away shouldn't make sense on other protocols) on network.c gives me twitches.


I like abstraction too: so don't make it a protocol-specific return code. Instead, it could be a generic "You should call proto_send()" thing rather than a protocol-specific "You should call obfs2_send()" thing.

Having the listeners have a list of connections just seems wrong, if the whole point is to serve as a socks_state-to-connection map. If the protocol send and recv methods really truly need access to the conn_t, then let's just pass them the conn_t! The functions that call proto_send and proto_recv have the conn_t already: no need to make the functions they call hunt for it.

You are right, doing all that just to get a socks_state-to-connection map is stupid. I just found it intuitive in the sense that the point of a listener is to accept connections and that would give us a way to go from a listener to a connection.

Passing conn_t to the protocols is ugly as well, but it provides us with ways to solve other similar problems (like #3291).

One thing I dislike about the passing conn_t solution is that it makes the protocols harder to test: it is much easier to make sure that they do the right things with evbuffers than it is to make sure that they do the right things with bufferevents.

Alternatively, we _could_ just make the protocol state contain a pointer to the evbuffer that we want to flush pending data into when the handshake completes. That's a hack too, but at least it isolates everything into the protocol layer.

comment:7 in reply to:  6 Changed 8 years ago by asn

Status: newneeds_review

Replying to nickm:

Replying to asn:

Replying to nickm:

I really prefer the solution I outlined where obfs2_recv() can return something that indicates that the caller needs to do an obfs2_send() right away without waiting any data.

Yes, I like that solution myself, but the implementation seems dirtier than it sounds on words. Basically, the caller of obfs2_send() is proto_send(). proto_send() can do the same things that obfs2_send() can do, so we have to go back to plaintext_read_cb(). We can indeed call obfs2_send() correctly from there, but isn't it ugly?
I know I'm stuck on the concept of abstraction when we just have one protocol, but still defining a protocol specific return code (since calling proto_send() right away shouldn't make sense on other protocols) on network.c gives me twitches.


I like abstraction too: so don't make it a protocol-specific return code. Instead, it could be a generic "You should call proto_send()" thing rather than a protocol-specific "You should call obfs2_send()" thing.

Having the listeners have a list of connections just seems wrong, if the whole point is to serve as a socks_state-to-connection map. If the protocol send and recv methods really truly need access to the conn_t, then let's just pass them the conn_t! The functions that call proto_send and proto_recv have the conn_t already: no need to make the functions they call hunt for it.

You are right, doing all that just to get a socks_state-to-connection map is stupid. I just found it intuitive in the sense that the point of a listener is to accept connections and that would give us a way to go from a listener to a connection.

Passing conn_t to the protocols is ugly as well, but it provides us with ways to solve other similar problems (like #3291).

One thing I dislike about the passing conn_t solution is that it makes the protocols harder to test: it is much easier to make sure that they do the right things with evbuffers than it is to make sure that they do the right things with bufferevents.

Alternatively, we _could_ just make the protocol state contain a pointer to the evbuffer that we want to flush pending data into when the handshake completes. That's a hack too, but at least it isolates everything into the protocol layer.

bug3302 branch fixes this by, indeed, setting a special return code for exactly this purpose! And I also think I'm not too sad about this solution any more.

comment:8 Changed 8 years ago by asn

Fixed stuff pointed out in comment:5:ticket:3291!
Thanks!

comment:9 Changed 8 years ago by nickm

merged; thanks!

comment:10 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:11 Changed 8 years ago by arma

Component: Pluggable transportObfsproxy
Note: See TracTickets for help on using tickets.