proxmox-mirrors/pve-http-server
1
0
Fork 0
mirror of https://git.proxmox.com/git/pve-http-server synced 2025-05-02 00:15:30 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

accept-phase: fix conn_count "leak"

Browse Source 
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>
This commit is contained in:
Stoiko Ivanov 2020-12-10 15:02:49 +01:00 committed by Thomas Lamprecht
parent 26e73e6b9e
commit c82d1707ed
1 changed files with 17 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
PVE/APIServer/AnyEvent.pm
View File

@ -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;
}