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

[Mark Ellzey <mthomas@xxxxxxxxxx>]

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. 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. 

> [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.
> 
> Instead, I think what we do want to do is make sure that
> consider_reading gets called on the *next* iteration of the event
> loop, as it would if this were a non-filtering bufferevent.  But I
> don't know that there's a good way to do that in the 2.0.x framework.
> 
> I think instead our best bet is to try to return the
> filtering-bufferevent behavior here as close as we can to the status
> quo ante before 2.0.16 came out, and to work on a real correct
> solution to this (and other tricky issues) for 2.1, where we can
> afford to do stuff like "build new infrastructure" and "refactor all
> the ugly code."
> 

I disagree with a complete revert. I wrote this patch to fix what I
consider a problem.

Prior to this patch if you had a openssl bev running in a deferred
state, the following would happen:

while (enabled && get_length(input) < wm->high) {
	int nt_o_read = READ_DEFAULT;

	r = do_read(bev, n_to_read);

	if (r <= 0) 
		break;
}

Now lets say you are pushing 2GB of data over that SSL socket, it calls
do_read(). If the SSL_read() was successful, the buffer is committed and
_bufferevent_run_readcb(bev) is called. Without deferred enabled, you
immediately get your readcb executed. But with deferred, the event is
queued until it can drop back into the event loop. 

The only way to get back to the event loop is for SSL_read to return <= 0. 
In the case of a bulk stream of input (like 2GB), libevent spin-locks inside
consider_reading; during which the input buffer grows, potentially causing 
massive resource exhaustion.  

If you want to wait until we can "build new infrastructure" and
"refactor", then there is no reason *not* to put this patch in place
until the refactoring.


Just as a side note, these problems really only exist because of OpenSSL. 
With normal sockets we can usually expect to read MTU minus L1-L3 overhead,
giving us plenty of leeway to do other things.
***********************************************************************
To unsubscribe, send an e-mail to majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.