Re: [pve-devel] [PATCH http-server v2 2/5] accept-phase: fix conn_count "leak"

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 04.12.20 18:56, Stoiko Ivanov wrote:
> When handling new connections in 'accept_connections' the number of
> active connections got increased before the AnyEvent::Handle
> registered the callback which would decrement it on error/eof.
> 
> Any error/die beforehand would skip the decrement, and leave the
> process in an endless loop upon exiting in wait_end_loop.
> 
> This can happen e.g. when the call to getpeername fails, or if the
> connection is denied by the ALLOW_FROM/DENY_FROM settings in
> '/etc/default/pveproxy' (which is also a simple reproducer for
> that).
> 
> Additionally it can cause a denial of service, by attempting to
> connect from a denied ip until the connection count exeeds the maximum
> connections of all child-processes.
> 
> Should the connection count become negative a warning is logged in both
> places where we decrement it, in case of a failed AnyEvent::Handle->new(),
> the count is not decremented if this would happen.
> 
> Reported via our community-forum:
> https://forum.proxmox.com/threads/pveproxy-eats-available-ram.79617/
> 
> Co-Authored-by: Dominik Csapak <d.csapak@proxmox.com>
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  PVE/APIServer/AnyEvent.pm | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
> index 7916bdd..af2fde8 100644
> --- a/PVE/APIServer/AnyEvent.pm
> +++ b/PVE/APIServer/AnyEvent.pm
> @@ -157,6 +157,11 @@ sub client_do_disconnect {
>  
>      &$shutdown_hdl($hdl);
>  
> +    if ($self->{conn_count} <= 0) {
> +	my $msg = "connection count <= 0!\n";
> +	syslog('warn', $msg);
> +	$self->dprint("warn: $msg");

see below, at the end, for a comment about doing both, debug print and syslog.

> +    }
>      $self->{conn_count}--;
>  
>      print "$$: CLOSE FH" .  $hdl->{fh}->fileno() . " CONN$self->{conn_count}\n" if $self->{debug};
> @@ -1489,8 +1494,6 @@ sub accept {
>  
>      fh_nonblocking $clientfh, 1;
>  
> -    $self->{conn_count}++;
> -
>      return $clientfh;
>  }
>  
> @@ -1564,6 +1567,7 @@ sub check_host_access {
>  sub accept_connections {
>      my ($self) = @_;
>  
> +    my $hdl_err;

can we please avoid adding to that mess of arbitrary shortened variable names, at
least for new ones?

Also, this suggest a bit like it would denote if there's an handle error, but what
it actually tells is, if handle creation point got reached - naming should not be bound
to much to one use case.

Maybe:
my $handle_creation = 0;

or rather, just reuse $early_err (which has similar naming issues, but is pre-existing),
the $self->push_request_header is doing everything in a eval + warn only scope anyway.

>      eval {
>  
>  	while (my $clientfh = $self->accept()) {
> @@ -1571,7 +1575,7 @@ sub accept_connections {
>  	    my $reqstate = { keep_alive => $self->{keep_alive} };
>  
>  	    # stop keep-alive when there are many open connections
> -	    if ($self->{conn_count} >= $self->{max_conn_soft_limit}) {
> +	    if ($self->{conn_count} + 1 >= $self->{max_conn_soft_limit}) {
>  		$reqstate->{keep_alive} = 0;
>  	    }
>  
> @@ -1587,6 +1591,8 @@ sub accept_connections {
>  		next;
>  	    }
>  
> +	    $hdl_err = 1;
> +	    $self->{conn_count}++;

still no comment whatsoever, that is unacceptable to me for such subtle thing, especially
paired with the commit message, which still mentions that increasing the count before
registering the callback handle (which we still do, for good reasons), makes it
confusing.

Please note that we explicitly increment here, not after calling ->new, because creating the
handle below starts off reading from the fh immediately and can trigger at least an on_error
(and possible other) callback.

>  	    $reqstate->{hdl} = AnyEvent::Handle->new(
>  		fh => $clientfh,
>  		rbuf_max => 64*1024,
> @@ -1609,6 +1615,7 @@ sub accept_connections {
>  		    if (my $err = $@) { syslog('err', "$err"); }
>  		},
>  		($self->{tls_ctx} ? (tls => "accept", tls_ctx => $self->{tls_ctx}) : ()));
> +	    $hdl_err = 0;
>  
>  	    print "$$: ACCEPT FH" .  $clientfh->fileno() . " CONN$self->{conn_count}\n" if $self->{debug};
>  
> @@ -1618,6 +1625,15 @@ sub accept_connections {
>  
>      if (my $err = $@) {
>  	syslog('err', $err);
> +	if ($hdl_err) {
> +	    if ($self->{conn_count} <= 0) {
> +		my $msg = "connection count <= 0 not decrementing!\n";
> +		syslog('warn', $msg);
> +		$self->dprint("warn: $msg");

If we already log to syslog, isn't that enough to have it visible?

Or could a single `warn` call do the "correct thing", i.e., stderr so to
syslog if running as systemd service and to console if running in foreground?

Not too hard feelings here though.

> +	    } else {
> +		$self->{conn_count}--;
> +	    }
> +	}
>  	$self->{end_loop} = 1;
>      }
>  
>