obfs2 delays sending pending data

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 (closed)) 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 (closed), 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)

added component::archived/obfsproxy owner::asn priority::medium resolution::fixed status::closed type::defect labels

Trac:
Component: - Select a component to Pluggable transport

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 (closed)) 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 (closed), 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?

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 (closed), 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();

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.

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 (closed)).

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 (closed)).

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.

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 (closed)).

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.

Trac:
Status: new to needs_review

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

merged; thanks!

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

Trac:
Component: Pluggable transport to Obfsproxy

closed