From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <s.ivanov@proxmox.com>
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 3077D6B0CE
for <pve-devel@lists.proxmox.com>; Thu, 10 Dec 2020 15:03:43 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 2759716245
for <pve-devel@lists.proxmox.com>; Thu, 10 Dec 2020 15:03:13 +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 8882316236
for <pve-devel@lists.proxmox.com>; Thu, 10 Dec 2020 15:03:11 +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 5451D44F47
for <pve-devel@lists.proxmox.com>; Thu, 10 Dec 2020 15:03:11 +0100 (CET)
From: Stoiko Ivanov <s.ivanov@proxmox.com>
To: pve-devel@lists.proxmox.com
Date: Thu, 10 Dec 2020 15:02:49 +0100
Message-Id: <20201210140251.6127-3-s.ivanov@proxmox.com>
X-Mailer: git-send-email 2.20.1
In-Reply-To: <20201210140251.6127-1-s.ivanov@proxmox.com>
References: <20201210140251.6127-1-s.ivanov@proxmox.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.077 Adjusted score from AWL reputation of From: address
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
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: [pve-devel] [PATCH http-server v3 2/4] 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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Thu, 10 Dec 2020 14:03:43 -0000
When handling new connections in 'accept_connections' the number of
active connections (conn_count) got increased before the callback, which
would eventually decrease it got registered in AnyEvent::Handle->new.
Any error/die before registering the callback 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.
This patch addresses the issue by incrementing the connection count
before attempting to create the handle, and decrementing it again, if
handle creation fails.
A warning is logged if 'conn_count' turns negative when decrementing
during cleanup on error/eof. In case creating a new handle during
initial accept_connection fails, a warning is logged as well, but
'conn_count' is not decremented.
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 | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
index b8c28ce..be60f2e 100644
--- a/PVE/APIServer/AnyEvent.pm
+++ b/PVE/APIServer/AnyEvent.pm
@@ -157,6 +157,8 @@ sub client_do_disconnect {
&$shutdown_hdl($hdl);
+ warn "connection count <= 0!\n" if $self->{conn_count} <= 0;
+
$self->{conn_count}--;
$self->dprint("CLOSE FH" . $hdl->{fh}->fileno() . " CONN$self->{conn_count}");
@@ -1489,8 +1491,6 @@ sub accept {
fh_nonblocking $clientfh, 1;
- $self->{conn_count}++;
-
return $clientfh;
}
@@ -1564,6 +1564,7 @@ sub check_host_access {
sub accept_connections {
my ($self) = @_;
+ my $handle_creation;
eval {
while (my $clientfh = $self->accept()) {
@@ -1571,7 +1572,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 +1588,11 @@ sub accept_connections {
next;
}
+ # Increment conn_count before creating new handle, since creation
+ # triggers callbacks, which can potentialy decrement (e.g.
+ # on_error) conn_count before AnyEvent::Handle->new() returns.
+ $handle_creation = 1;
+ $self->{conn_count}++;
$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}) : ()));
+ $handle_creation = 0;
$self->dprint("ACCEPT FH" . $clientfh->fileno() . " CONN$self->{conn_count}");
@@ -1618,6 +1625,13 @@ sub accept_connections {
if (my $err = $@) {
syslog('err', $err);
+ if ($handle_creation) {
+ if ($self->{conn_count} <= 0) {
+ warn "connection count <= 0 not decrementing!\n";
+ } else {
+ $self->{conn_count}--;
+ }
+ }
$self->{end_loop} = 1;
}
--
2.20.1