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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id EF5536218A for ; Fri, 4 Dec 2020 08:41:37 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DF77AAE0A for ; Fri, 4 Dec 2020 08:41:37 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id EC79AADFE for ; Fri, 4 Dec 2020 08:41:36 +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 9696B44CC7 for ; Fri, 4 Dec 2020 08:41:36 +0100 (CET) To: Proxmox VE development discussion , Stoiko Ivanov References: <20201203184322.20253-1-s.ivanov@proxmox.com> <20201203184322.20253-2-s.ivanov@proxmox.com> From: Thomas Lamprecht Message-ID: <9b796a6a-71ff-5e51-101f-15fce02240e7@proxmox.com> Date: Fri, 4 Dec 2020 08:41:35 +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: <20201203184322.20253-2-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.073 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment KAM_SHORT 0.001 Use of a URL Shortener for very short URL 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, anyevent.pm, redd.it] Subject: Re: [pve-devel] [PATCH http-server 1/3] accept-phase: fix conn_count "leak" 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: Fri, 04 Dec 2020 07:41:38 -0000 On 03.12.20 19:43, 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. >=20 > Any error/die beforehand would skip the decrement, and leave the > process in an endless loop upon exiting in wait_end_loop. >=20 > 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 the simplest reproducer for > that). >=20 > 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. >=20 > Reported via our community-forum: > https://forum.proxmox.com/threads/pveproxy-eats-available-ram.79617/ >=20 > Co-Authored-by: Dominik Csapak > Signed-off-by: Stoiko Ivanov > --- > PVE/APIServer/AnyEvent.pm | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) >=20 > diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm > index c55da7f..c5f5fdc 100644 > --- a/PVE/APIServer/AnyEvent.pm > +++ b/PVE/APIServer/AnyEvent.pm > @@ -1479,8 +1479,6 @@ sub accept { > =20 > fh_nonblocking $clientfh, 1; > =20 > - $self->{conn_count}++; > - > return $clientfh; > } > =20 > @@ -1561,7 +1559,7 @@ sub accept_connections { > my $reqstate =3D { keep_alive =3D> $self->{keep_alive} }; > =20 > # stop keep-alive when there are many open connections > - if ($self->{conn_count} >=3D $self->{max_conn_soft_limit}) { > + if ($self->{conn_count}+1 >=3D $self->{max_conn_soft_limit}) { style nit: don't glue operators together `self->{conn_count} + 1` > $reqstate->{keep_alive} =3D 0; > } > =20 > @@ -1600,6 +1598,9 @@ sub accept_connections { > }, > ($self->{tls_ctx} ? (tls =3D> "accept", tls_ctx =3D> $self->{tls_ctx= }) : ())); > =20 > + $self->{conn_count}++; > + But isn't this wrong too? The FH could already get a EOF here, and thus g= et reduced before increased - one could maybe argue "well it should get increased ag= ain after, here, so brought in sync again", i.e.: 1. Get's registered 2. clientfh EOF -> $self->client_do_disco= nnect($reqstate); -> $self->{conn_count}--;= ! Wrong counter here, could lead to possible wrong decisions now already = (not checked for sure) or when adding/changing something (as this is non-obvious, not = even a comment hinting it!) 3. resume here, brought in sync again, reminds me of a short comic strip = I recently run into [0]. So between 2. and 3. we are in limbo, while short it still matters, every= race triggers sooner or later, computers are just to fast and scheduling to nondetermin= istic for that to not happen. Why not move the $self->{conn_count}++; before AnyEvent Handle instance i= s created, i.e., where we do $early_err =3D 0; as this effectively is the barrier fo= r the connection being valid or not. We could also add handling for when the handle creati= on itself fails, setting a flag afterwards and checking both, that flag and $early_err in = the existing error handling branch outside of the eval, and decrement in that case. Or, do you have some documented behavior, not stated here in the commit, = that this all just cannot happen at all? [0]: https://i.redd.it/m4zbw3u7rbk21.jpg > + > print "$$: ACCEPT FH" . $clientfh->fileno() . " CONN$self->{conn= _count}\n" if $self->{debug}; > =20 > $self->push_request_header($reqstate); >=20