connection_write_to_buf: indirect recursion for CONN_TYPE_CONTROL

changed milestone to %Tor: 0.2.7.x-final

added TorCoreTeam201508 component::core tor/tor milestone::Tor: 0.2.7.x-final owner::nickm priority::medium resolution::fixed sponsorS status::closed type::defect labels

Not sure about sanity of 2 and 3 way either. Only proper way to escape recursion is to separate writing to outbuf and flushing of outbuf by removing ability for immediate flush of outbuf. Do we really need to flush it such way for control port:

               conn->outbuf_flushlen-len < 1<<16 &&
               conn->outbuf_flushlen >= 1<<16

what is profit, when it happens, why it happens?

Trac:
Milestone: N/A to Tor: 0.2.7.x-final

Yet

      log_warn(LD_NET,
               "write_to_buf failed. Closing connection (fd %d).",
               (int)conn->s);

for connection_write_to_buf, no need flushing even. Can't to be fixed by suggested fix.

Replying to cypherpunks:

Do we really need to flush it such way for control port: {{{ conn->outbuf_flushlen-len < 1<<16 && conn->outbuf_flushlen >= 1<<16 }}} what is profit, when it happens, why it happens?

Oh, oh, I know this one.

Back in the day, Vidalia would ask a control port question with a really big answer (like "give me all the microdescs"), and Tor would put the answer on its control port, but then the outbuf would get too big, and the "is there too much on the outbuf" check would trigger, killing the connection.

Maybe enough of the rest of the code has changed that this bad behavior wouldn't happen anymore? Or maybe it is just like it was?

Oh, oh, I know this one.

Back in the day, Vidalia would ask a control port question with a really big answer (like "give me all the microdescs"), and Tor would put the answer on its control port, but then the outbuf would get too big, and the "is there too much on the outbuf" check would trigger, killing the connection.

Maybe enough of the rest of the code has changed that this bad behavior wouldn't happen anymore? Or maybe it is just like it was?

Structure for outbuf was changes since then, there are no need for connection's buf size limits anymore and code do not closes connection for such reason anymore.

But comment for ab838bddb89fa3f37eeada29e8c10d16873b5e86 says:

Flush local controller connection buffers periodically as we're writing to them, so we avoid queueing 4+ megabytes of data before trying to flush.

Is it still possible to queue so many bytes for control connection, why?

Trac:
Keywords: N/A deleted, TorCoreTeam201507 added

Refactoring this is a testability improvement

Trac:
Keywords: TorCoreTeam201507 deleted, sponsorS, TorCoreTeam201508 added
Status: new to accepted
Owner: N/A to nickm

I think we can remove that entire block from connection_write_to_buf_impl_. So, way #1 it is. Roger, does anything bad happen if we take this out? I just tried shoving a huge pile of stuff onto an output buffer, and everything was fine.