From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 9B96A69CAD for ; Mon, 7 Dec 2020 11:40:20 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 87A02231B9 for ; Mon, 7 Dec 2020 11:39:50 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id F36DB231AF for ; Mon, 7 Dec 2020 11:39:49 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B6CC244E4B for ; Mon, 7 Dec 2020 11:39:49 +0100 (CET) To: Proxmox VE development discussion , Stoiko Ivanov References: <20201204175629.30116-1-s.ivanov@proxmox.com> <20201204175629.30116-4-s.ivanov@proxmox.com> From: Thomas Lamprecht Message-ID: Date: Mon, 7 Dec 2020 11:39:48 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:84.0) Gecko/20100101 Thunderbird/84.0 MIME-Version: 1.0 In-Reply-To: <20201204175629.30116-4-s.ivanov@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.070 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH http-server v2 3/5] accept-phase: shutdown socket on early error X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Dec 2020 10:40:20 -0000 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. >=20 > Co-Authored-by: Dominik Csapak > Signed-off-by: Stoiko Ivanov > --- > PVE/APIServer/AnyEvent.pm | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) >=20 > 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 { > =20 > my $cip =3D Net::IP->new($clientip); > =20 > + if (!$cip) { > + self->dprint("client IP not parsable: $@"); > + return 0; > + } > + > my $match_allow =3D 0; > my $match_deny =3D 0; > =20 > @@ -1567,10 +1572,13 @@ sub check_host_access { > sub accept_connections { > my ($self) =3D @_; > =20 > - my $hdl_err; > + my ($clientfh, $early_err, $hdl_err); ah OK, ignore my regards to "$early_err" in the previous comment, I thoug= ht it was pre-exsiting... > eval { > =20 > - while (my $clientfh =3D $self->accept()) { > + while (1) { > + $early_err =3D 1; > + $clientfh =3D $self->accept(); > + last if !$clientfh; what use has above change? Why not keeping it as is, you can still declar= e $clientfh earlier to extend it's scope: > + while ($clientfh =3D $self->accept()) { > =20 > my $reqstate =3D { keep_alive =3D> $self->{keep_alive} }; > =20 > @@ -1582,14 +1590,19 @@ sub accept_connections { > if (my $sin =3D getpeername($clientfh)) { > my ($pfamily, $pport, $phost) =3D PVE::Tools::unpack_sockaddr_in46($= sin); > ($reqstate->{peer_port}, $reqstate->{peer_host}) =3D ($pport, Socke= t::inet_ntop($pfamily, $phost)); > + } else { > + shutdown($clientfh, 1); Do we still plan to send anything? I.e., was `1` (SHUT_RD) used because o= f caution or are there more explicit reasons for not using `2` (SHUT_RDWR)? Can be fin= e, but would be good to know. > + next; > } > =20 > 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} =3D 403; > $self->log_request($reqstate); > + shutdown($clientfh, 1); same as above > next; > } > + $early_err =3D 0; > =20 > $hdl_err =3D 1; > $self->{conn_count}++; > @@ -1625,6 +1638,7 @@ sub accept_connections { > =20 > if (my $err =3D $@) { > 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, r= educing the combination matrix a bit. > if ($hdl_err) { > if ($self->{conn_count} <=3D 0) { > my $msg =3D "connection count <=3D 0 not decrementing!\n"; >=20