Re: [pve-devel] [PATCH http-server v2 3/5] accept-phase: shutdown socket on early error

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

On 04.12.20 18:56, Stoiko Ivanov wrote:
> if an error happens before AnyEvent::Handle registers the cleanup
> callback, we should shutdown the socket, when handling it.
> 
> Co-Authored-by: Dominik Csapak <d.csapak@proxmox.com>
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  PVE/APIServer/AnyEvent.pm | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
> index af2fde8..a679006 100644
> --- a/PVE/APIServer/AnyEvent.pm
> +++ b/PVE/APIServer/AnyEvent.pm
> @@ -1535,6 +1535,11 @@ sub check_host_access {
>  
>      my $cip = Net::IP->new($clientip);
>  
> +    if (!$cip) {
> +	self->dprint("client IP not parsable: $@");
> +	return 0;
> +    }
> +
>      my $match_allow = 0;
>      my $match_deny = 0;
>  
> @@ -1567,10 +1572,13 @@ sub check_host_access {
>  sub accept_connections {
>      my ($self) = @_;
>  
> -    my $hdl_err;
> +    my ($clientfh, $early_err, $hdl_err);

ah OK, ignore my regards to "$early_err" in the previous comment, I thought it was pre-exsiting...

>      eval {
>  
> -	while (my $clientfh = $self->accept()) {
> +	while (1) {
> +	    $early_err = 1;
> +	    $clientfh = $self->accept();
> +	    last if !$clientfh;

what use has above change? Why not keeping it as is, you can still declare $clientfh
earlier to extend it's scope:

> +	while ($clientfh = $self->accept()) {


>  
>  	    my $reqstate = { keep_alive => $self->{keep_alive} };
>  
> @@ -1582,14 +1590,19 @@ sub accept_connections {
>  	    if (my $sin = getpeername($clientfh)) {
>  		my ($pfamily, $pport, $phost) = PVE::Tools::unpack_sockaddr_in46($sin);
>  		($reqstate->{peer_port}, $reqstate->{peer_host}) = ($pport,  Socket::inet_ntop($pfamily, $phost));
> +	    } else {
> +		shutdown($clientfh, 1);

Do we still plan to send anything? I.e., was `1` (SHUT_RD) used because of caution or
are there more explicit reasons for not using `2` (SHUT_RDWR)? Can be fine, but would
be good to know.

> +		next;
>  	    }
>  
>  	    if (!$self->{trusted_env} && !$self->check_host_access($reqstate->{peer_host})) {
>  		print "$$: ABORT request from $reqstate->{peer_host} - access denied\n" if $self->{debug};
>  		$reqstate->{log}->{code} = 403;
>  		$self->log_request($reqstate);
> +		shutdown($clientfh, 1);

same as above

>  		next;
>  	    }
> +	    $early_err = 0;
>  
>  	    $hdl_err = 1;
>  	    $self->{conn_count}++;
> @@ -1625,6 +1638,7 @@ sub accept_connections {
>  
>      if (my $err = $@) {
>  	syslog('err', $err);
> +	shutdown($clientfh, 1) if $early_err || $hdl_err;

same as above, and maybe we could do with just one such flag variables, reducing the combination
matrix a bit.

>  	if ($hdl_err) {
>  	    if ($self->{conn_count} <= 0) {
>  		my $msg = "connection count <= 0 not decrementing!\n";
>