[sslh] [PATCH 02/10] Let defer_write accumulate data

[ondra+sslh at mistotebe.net]

On Tue, Sep 24, 2013 at 08:46:59AM +0200, Yves Rutschle wrote:
> Warning, morning coffee not yet digested, so I may be
> missing things :-)
> 
> I have issues with this one. The intent of my code was:
> - begin_defered_data contains malloced pointer to memory
>   area, and doesn't move
> - defered_data contains pointer to the beginning of data yet
>   to be written
> - defered_data_size contains number of bytes left to write
> 
> Hence, the flush_defered() function only works on
> defered_data and defered_data_size, and we can simply free
> begin_defered_data when we get to defered_data_szie == 0.
> (In case you missed it, the whole defered_write system is
> used in sslh-select when write() would block, not just at
> connection start-up).

Hmm, I thought you make sure in sslh-select that you drain the entire
buffer before trying to read next time. Indeed every time there is a
short write, FD_STALLED is returned and the read side of the connection
is stopped until the buffer has been drained.

If that did not hold, wouldn't fd2fd be broken (both before the patch
and after)? It would first write the newly read data, reordering the
stream.

Also, defer_write was not able add more data to the buffer before the
patch, causing a memory leak instead. After the patch, if invoked on a
partially drained buffer, defer_write should trigger a libc assert as
the pointer to reallocate was never returned by (m/re)alloc.

I was thinking about getting rid of that invariant, but I don't think it
affects the performance much, and without it the source would probably
be much harder to reason about.

> In your code, I don't think you can realloc on
> q->defered_data, as this could have been changed by
> flush_defer(), and I don't think you can use
> defered_data_size, for the same reason (unless you want to
> drop the head of the buffer if it happens to have been
> written, but I think that will result in memcpy'ing the
> left-over data every time).

I agree that reallocation should have been done on begin_defered_data
to be consistent (and for now, an assert that begin_defered_data ==
defered_data added).