Prop 174: Optimistic Data for Tor: Server Side
Attached is Ian's patch to implement the server side of proposal 174. The goal is to accept and queue DATA cells received at an exit when the connection is not yet opened.
Notes from Ian:
The current code actually correctly handles queued data at the Exit; if there is queued data in a EXIT_CONN_STATE_CONNECTING stream, that data will be immediately sent when the connection succeeds. If the connection fails, the data will be correctly ignored and freed. The problem with the current server code is that the server currently drops DATA cells on streams in the EXIT_CONN_STATE_CONNECTING state. Also, if you try to queue data in the EXIT_CONN_STATE_RESOLVING state, bad things happen because streams in that state don't yet have conn->write_event set, and so some existing sanity checks (any stream with queued data is at least potentially writable) are no longer sound.
The solution is to simply not drop received DATA cells while in the EXIT_CONN_STATE_CONNECTING state. Also do not send SENDME cells in this state, so that the OP cannot send more than one window's worth of data to be queued at the Exit. Finally, patch the sanity checks so that streams in the EXIT_CONN_STATE_RESOLVING state that have buffered data can pass.
[...] Here is a simple patch. It seems to work with both regular streams and hidden services, but there may be other corner cases I'm not aware of. (Do streams used for directory fetches, hidden services, etc. take a different code path?)
Activity
Trac:
prop174_v1.diffTrac:
Cc: N/A to iang@cs.uwaterloo.caSo, piece by piece, initial thoughts.
connection.c:
The check for conn->state seems wrong, since the connection state's values are relative to the connection's type. Only if conn->type ==CONN_TYPE_EXIT is it meaningful to say "conn->state == EXIT_CONN_STATE_anything".
On the other hand, if what we're really concerned about is the possible absence of conn->write_event, we could as easily check for conn->write_event rather than the state.
The same note applies to the second check for conn->state in the tor_assert below.
relay.c:
Rather than special-casing the code that called connection_edge_process_relay_cell_not_open(), could we not move the new handling for DATA cells into connection_edge_process_relay_cell_not_open() ? Or would that make stuff ugly? (Again, you need to check conn->type before conn->state is meaningful)
' log_warn(domain, "Optimistic data received."); ' is debugging code.
I worry that making the consider_sending_sendme() calls conditional might put us in a position where they could never be called. If a client exhausts the window of a sendme cell before the resolve and connect are done, will a sendme eventually be sent? If so, what calls it?
You're totally right about checking for CONN_TYPE_EXIT, of course. I must have been confused by a quick glance at or.h and seeing that the EXIT and AP states were disjoint, and not noticing that the others are not.
Checking for write_event in chunk 1 is also a plausible solution; in chunk 2, I think the state check (suitably corrected to also check for CONN_TYPE_EXIT) is the right choice.
I don't think we want to move code into connection_edge_process_relay_cell_not_open(), since the patch causes control to skip that call, and fall through to the RELAY_COMMAND_DATA handling below. All that stuff would have to be copied into connection_edge_process_relay_cell_not_open(), which seems poor.
Agreed on the debugging code; sorry that slipped in.
Isn't it the case that when the cells get sent to the server, it will trigger the check to see if SENDMEs should be sent? SENDMEs are sent when the buffer empties, not when it fills, right? I expect that not wrapping these calls would have the same (i.e. no) effect, but it seems cleaner to me to not check if you should send a SENDME if you know you won't.
Trac:
Parent: N/A to #1849 (moved)Okay. Hm. In the meantime, merging the bufferevent stuff has made it so that checking conn->write_event doesn't make sense for bufferevent-based connections. I'll need to look into that and figure out the right approach. Further, if a bufferevent-based connection is in STATE_RESOLVING, I am not sure that it actually has a bufferevent object to queue the outgoing data on yet. We should check that out.
A question making the circuit_consider_sending_sendme() code conditional on "!optimistic_data": suppose that the client sends enough optimistic data for the server to exhaust its window, then the client stops sending data before the connection is complete. In this case, will the server ever send the client SENDME cell?
