Re: [Libevent-users] PATCH: bufferevent_openssl consider_read doesn't drain underlying BE in all cases

[Nick Mathewson <nickm@xxxxxxxxxxxxx>]

On Mon, Dec 19, 2011 at 11:00 PM, Mark Ellzey <mthomas@xxxxxxxxxx> wrote:
> On Mon, Dec 19, 2011 at 09:57:08PM -0500, Nick Mathewson wrote:
>> I agree with the part of your fix where we don't ignore the extra data
>> whose presence is indicated by a nonzero  SSL_pending.
>>
>> But I think these fixes all have some problem(s) in common:  They
>> ignore the reason for the logic that initially sets n_to_read, and
>> instead have the behavior for the filtering SSL bufferevent be "read
>> aggressively, as much as possible!"
>>
>> That isn't correct behavior.  Because of rate-limiting[1],
>> watermarks[2], fairness/starvation[3], and loop-avoidance[4], we
>> *don't* want to read aggressively from a single underlying bufferevent
>> until it's out of data.
>>
>> [1][2] This behavior can make the SSL bufferevent aggressively overrun
>> rate limits and watermarks.
>>
>
> IMHO, SSL + rate-limiting is going to be screwy no matter what. AFIAK
> OpenSSL does not have a good way to inform the user just how much total
> data was read before decryption.

Actually, it does; the BIO_number_written and BIO_number_read
functions return the total number of bytes written/read on a BIO.

> We cannot assume the sizes either.
> (think SSL_MODE_RELEASE_BUFFERS)
>
> Watermarks, on the other hand, I agree. We should give a hard look at
> how we can fix this.

Here's what I think will work: Instead of setting the n_to_read based
on the number of bytes currently in the underlying bufferevent (as in
your fix), we only do so if reading is enabled, reading is not
suspended, and the watermark is not overrun.

Why is that safe?  Because each of these conditions causes
consider_reading to get called again once it is no longer triggered.
If reading is enabled again, be_openssl_enable will call
consider_reading.  If reading is unsuspended,
bufferevent_unsuspend_read will call be_openssl_enable, etc.  And if
the buffer is drained below its watermark, then
bufferevent_inbuf_wm_cb calls bufferevent_wm_unsuspend_read, which
then calls bufferevent_unsuspend_read, etc.

So the only case where we want to immediately try reading more from
the underlying buffer is when there is more data to read, and nothing
about our current status prevents us from trying to read it.

What's more, we already have a function that returns a reasonable
amount of data to read, but returns 0 in the cases above:
bytes_to_read().

I've commited a possible patch as 5b4b8126de52c; can folks please
test? I'd like to get a new stable release out soon if possible.

Now, this still has the problems below, but I think that we can
probably survive without fixing them in 2.0.x.  Doing SSL over a
paired bufferevent makes sense for testing, but is a little nutty.
The resource starvation issue is annoying, but the underlying
socket-based bufferevent should prevent stuff from getting _too_ bad
there.

>> [3] If a bunch of data has arrived on a the underlying bufferevent, we
>> probably don't want to aggressively do SSL on it until its gone
>> without giving other events a chance to fire.
>>
>> [4] If the underlying bufferevent is a paired bufferevent, draining it
>> could cause it to refill itself from its partner, invoking callbacks
>> that might refill the partner, etc.
-- 
Nick
***********************************************************************
To unsubscribe, send an e-mail to majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.