[pve-devel] [PATCH-SERIES http-server 0/3] fix disconnecting clients

[pve-devel] [PATCH-SERIES http-server 0/3] fix disconnecting clients

[Fiona Ebner]

Commit 07e56cc ("fix unexpected EOF for client when closing TLS
session") added a call to stoptls() before the call to shutdown() for
the handle's file descriptor. However, the documentation for
AnyEvent[0] mentions for stoptls():

> This method may invoke callbacks (and therefore the handle might be
> destroyed after it returns).

Indeed, the on_error callback might get invoked and lead to a
"detected empty handle" error message as reported in the community
forum [1].

Also, it is necessary to check that the handle is still defined
before calling shutdown(). Otherwise, this can result in a warning:

> Can't use an undefined value as a symbol reference at
> /usr/share/perl5/PVE/APIServer/AnyEvent.pm line 150.

as reported in the community forum [1] too.


Patches 1/3 and 2/3 are fixes for the reported issues. Patch 3/3 is
further future-proofing.


[0]: https://metacpan.org/pod/AnyEvent::Handle#$handle-%3Estoptls
[1]: https://forum.proxmox.com/threads/164744/


Fiona Ebner (3):
  anyevent: disconnect: check that handle is still defined before
    calling shutdown()
  anyevent: always avoid re-entering client_do_disconnect() in on_error
    callback
  anyevent: handle 'disconnected' flag in client_do_disconnect() itself

 src/PVE/APIServer/AnyEvent.pm | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

-- 
2.39.5



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

[pve-devel] [PATCH http-server 1/3] anyevent: disconnect: check that handle is still defined before calling shutdown()

[Fiona Ebner]

Commit 07e56cc ("fix unexpected EOF for client when closing TLS
session") added a call to stoptls() before the call to shutdown() for
the handle's file descriptor. However, the documentation for
AnyEvent[0] mentions for stoptls():

> This method may invoke callbacks (and therefore the handle might be
> destroyed after it returns).

Therefore, it is necessary to check that the handle is still defined
before calling shutdown(). Otherwise, this can result in a warning:

> Can't use an undefined value as a symbol reference at
> /usr/share/perl5/PVE/APIServer/AnyEvent.pm line 150.

as reported in the community forum [1].

The debug print message for closing the file handle is split up,
because part of it relies on the file handle to be defined.

[0]: https://metacpan.org/pod/AnyEvent::Handle#$handle-%3Estoptls
[1]: https://forum.proxmox.com/threads/164744/

Fixes: 07e56cc ("fix unexpected EOF for client when closing TLS session")
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/APIServer/AnyEvent.pm | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index 9b18ee2..3f8642b 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -146,8 +146,11 @@ sub client_do_disconnect {
 	$hdl->on_read(undef);
 	$hdl->on_eof(undef);
 
-	$hdl->stoptls();
-	shutdown($hdl->{fh}, 1);
+	$self->dprint("CLOSE FH" .  $hdl->{fh}->fileno());
+
+	$hdl->stoptls(); # can invoke callbacks and destroy the handle
+
+	shutdown($hdl->{fh}, 1) if defined($hdl) && defined($hdl->{fh});
     };
 
     if (my $proxyhdl = delete $reqstate->{proxyhdl}) {
@@ -170,7 +173,7 @@ sub client_do_disconnect {
 
     $self->{conn_count}--;
 
-    $self->dprint("CLOSE FH" .  $hdl->{fh}->fileno() . " CONN$self->{conn_count}");
+    $self->dprint("DISCONNECT CONN$self->{conn_count}");
 }
 
 sub finish_response {
-- 
2.39.5



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

[pve-devel] [PATCH http-server 2/3] anyevent: always avoid re-entering client_do_disconnect() in on_error callback

[Fiona Ebner]

Commit f737984 ("fix #4816: do not disconnect twice if client sends no
data") introduced a 'disconnected' flag in the request state to avoid
duplicate calls to client_do_disconnect() for a given client. This
works, except in the case where client_do_disconnect() enters the
on_error callback itself. To fix this, set the 'disconnected' flag
before calling client_do_disconnect().

This was exposed by commit 07e56cc ("fix unexpected EOF for client
when closing TLS session") which introduced a call to stoptls() in
client_do_disconnect(). The documentation [0] mentions for stoptls():

> This method may invoke callbacks (and therefore the handle might be
> destroyed after it returns).

Indeed, the on_error callback might get invoked and lead to a
"detected empty handle" error message as reported in the community
forum [1].

[0]: https://metacpan.org/pod/AnyEvent::Handle#$handle-%3Estoptls
[1]: https://forum.proxmox.com/threads/164744/

Fixes: 07e56cc ("fix unexpected EOF for client when closing TLS session")
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/APIServer/AnyEvent.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index 3f8642b..db14a7d 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -1915,8 +1915,8 @@ sub accept_connections {
 			# 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;
+			    $self->client_do_disconnect($reqstate);
 			}
 		    };
 		    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

[pve-devel] [PATCH http-server 3/3] anyevent: handle 'disconnected' flag in client_do_disconnect() itself

[Fiona Ebner]

Commit f737984 ("fix #4816: do not disconnect twice if client sends no
data") introduced a 'disconnected' flag in the request state to avoid
duplicate calls to client_do_disconnect() for a given client. The flag
is only set and checked in the on_error callback of the handle
however. Do this more centrally at the beginning of the
client_do_disconnect() function itself to catch all callers and code
paths that could lead to a duplicate call. For example, while not
currently known to cause issues, the on_eof handler might re-enter the
function.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/APIServer/AnyEvent.pm | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index db14a7d..b71a9a5 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -136,6 +136,13 @@ sub cleanup_reqstate {
 sub client_do_disconnect {
     my ($self, $reqstate) = @_;
 
+    # Avoid any re-entrant call. For example, the on_error callback can be called twice for the same
+    # connection/handle if the timeout is reached before any data has been received. The on_error
+    # callback might also get invoked as part of the stoptls() call during shutdown below, which is
+    # another situation where the function would be re-entered without this check.
+    return if $reqstate->{disconnected};
+    $reqstate->{disconnected} = 1;
+
     cleanup_reqstate($reqstate, 1);
 
     my $shutdown_hdl = sub {
@@ -1911,13 +1918,7 @@ sub accept_connections {
 		    my ($hdl, $fatal, $message) = @_;
 		    eval {
 			$self->log_aborted_request($reqstate, $message);
-			# 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}) {
-			    $reqstate->{disconnected} = 1;
-			    $self->client_do_disconnect($reqstate);
-			}
+			$self->client_do_disconnect($reqstate);
 		    };
 		    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

Re: [pve-devel] [PATCH-SERIES http-server 0/3] fix disconnecting clients

[Thomas Lamprecht]

On Tue, 08 Apr 2025 16:20:11 +0200, Fiona Ebner wrote:
> Commit 07e56cc ("fix unexpected EOF for client when closing TLS
> session") added a call to stoptls() before the call to shutdown() for
> the handle's file descriptor. However, the documentation for
> AnyEvent[0] mentions for stoptls():
> 
> > This method may invoke callbacks (and therefore the handle might be
> > destroyed after it returns).
> 
> [...]

Applied, thanks!

Btw. I did some benchmarks yesterday running `oha` [0] and I rechecked the logs
and I found a handful of such error messages too that I did overlook before.
With your patches I could not see them anymore after running a 10s five times
(vs just once yesterday)

[1/3] anyevent: disconnect: check that handle is still defined before calling shutdown()
      commit: 0ec627cff14a51695e6f81f86b34b398b07cf329
[2/3] anyevent: always avoid re-entering client_do_disconnect() in on_error callback
      commit: fa639169214f679c90274e4a6f1a84b864212326
[3/3] anyevent: handle 'disconnected' flag in client_do_disconnect() itself
      commit: f82b416214c94720c04dd02d5f5778af3439187f

[0]: https://github.com/hatoo/oha


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