[pve-devel] [PATCH http-server] fix #4816: do not disconnect twice if client sends no data

[pve-devel] [PATCH http-server] fix #4816: do not disconnect twice if client sends no data

[Fabian Grünbichler]

client_do_disconnect expects to be called exactly once per connection, since it
takes care of closing and unsetting the handle corresponding to the connection.
to find bugs in our connection handling, it will log "detected empty handle" if
it is called for a request/connection that no longer has a handle.

the edge case of opening a connection without sending any data leads to the
error callback being called twice:

Dec 04 09:37:02 xxx pveproxy[175235]: err (): Connection timed out

this is the (5 second) timeout triggering

Dec 04 09:37:02 xxx pveproxy[175235]: err (1): Broken pipe

this is AnyEvent trying to drain the buffer while the connection is already
closed

as soon as a single byte of traffic is sent, only the timeout will trigger.

there is no guarantee that the on_error callback is only called once (in fact,
it's possible to return from it for non-fatal errors and continue processing
the connection).

if there are further reports of empty handles with this in place, other
on_error callbacks might need similar logic - but it should only be added if
the triggering conditions are clear and deemed safe. the additional logging is
only cosmetic after all, but might point out an actual issue in our connection
handling code.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/APIServer/AnyEvent.pm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index 24209a1..6c165c8 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -1867,7 +1867,13 @@ sub accept_connections {
 		    my ($hdl, $fatal, $message) = @_;
 		    eval {
 			$self->log_aborted_request($reqstate, $message);
-			$self->client_do_disconnect($reqstate);
+			# this error callback can be called twice for the same
+			# connection/handle if the timeout is reached before
+			# any data has been received, avoid misleading errors
+			if (!$reqstate->{disconnected}) {
+			    $self->client_do_disconnect($reqstate);
+			    $reqstate->{disconnected} = 1;
+			}
 		    };
 		    if (my $err = $@) { syslog('err', "$err"); }
 		},
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel