public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH v3 storage] fix insecure migration failing if waiting on lock
@ 2024-04-17  9:48 Mira Limbeck
  2024-04-17 10:32 ` [pve-devel] applied: " Thomas Lamprecht
  0 siblings, 1 reply; 3+ messages in thread
From: Mira Limbeck @ 2024-04-17  9:48 UTC (permalink / raw)
  To: pve-devel

both STDOUT and STDERR are written into `$info` which is then parsed for
IP and port of the target socket listening.
when the ports file can't be locked immediately `trying to acquire
lock...` is printed on STDERR and in turn written into `$info`.
trying to parse the IP then fails, resulting in a migration or
replication failing.

the bare open3 call is replaced by the run_command wrapper from
pve-common to use a safe wrapper around open3 with the same
functionality.
STDERR is read separatey from STDOUT and the last line of STDERR is
kept in case of errors.

Fixes: 57acd6a ("fix #1452: also log stderr of remote command with
insecure storage migration")

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
v3:
 - added log prefix for remote error logs
 - fixed style issues

v2:
 - incorporated Fiona's suggestions
   - added `Fixes: ...` to commit message
   - kept old ip/port matching including # untaint comments
   - added logging for all messages in STDERR
   - simplified branches

 src/PVE/Storage.pm | 91 ++++++++++++++++++++++++++++------------------
 1 file changed, 55 insertions(+), 36 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 40314a8..ade4523 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -851,43 +851,62 @@ sub storage_migrate {
 
     eval {
 	if ($insecure) {
-	    my $input = IO::File->new();
-	    my $info = IO::File->new();
-	    open3($input, $info, $info, @$recv)
-		or die "receive command failed: $!\n";
-	    close($input);
-
-	    my $try_ip = <$info> // '';
-	    my ($ip) = $try_ip =~ /^($PVE::Tools::IPRE)$/ # untaint
-		or die "no tunnel IP received, got '$try_ip'\n";
-
-	    my $try_port = <$info> // '';
-	    my ($port) = $try_port =~ /^(\d+)$/ # untaint
-		or die "no tunnel port received, got '$try_port'\n";
-
-	    my $socket = IO::Socket::IP->new(PeerHost => $ip, PeerPort => $port, Type => SOCK_STREAM)
-		or die "failed to connect to tunnel at $ip:$port\n";
-	    # we won't be reading from the socket
-	    shutdown($socket, 0);
-
-	    eval { run_command($cmds, output => '>&'.fileno($socket), errfunc => $match_volid_and_log); };
-	    my $send_error = $@;
-
-	    # don't close the connection entirely otherwise the receiving end
-	    # might not get all buffered data (and fails with 'connection reset by peer')
-	    shutdown($socket, 1);
-
-	    # wait for the remote process to finish
-	    while (my $line = <$info>) {
-		$match_volid_and_log->("[$target_sshinfo->{name}] $line");
-	    }
+	    my $ip;
+	    my $port;
+	    my $socket;
+	    my $send_error;
+
+	    my $handle_insecure_migration = sub {
+		my $line = shift;
+
+		if (!$ip) {
+		    ($ip) = $line =~ /^($PVE::Tools::IPRE)$/ # untaint
+			or die "no tunnel IP received, got '$line'\n";
+		} elsif (!$port) {
+		    ($port) = $line =~ /^(\d+)$/ # untaint
+			or die "no tunnel port received, got '$line'\n";
+
+		    # create socket, run command
+		    $socket = IO::Socket::IP->new(
+			PeerHost => $ip,
+			PeerPort => $port,
+			Type => SOCK_STREAM,
+		    );
+		    die "failed to connect to tunnel at $ip:$port\n" if !$socket;
+		    # we won't be reading from the socket
+		    shutdown($socket, 0);
+
+		    eval {
+			run_command(
+			    $cmds,
+			    output => '>&'.fileno($socket),
+			    errfunc => $match_volid_and_log,
+			);
+		    };
+		    $send_error = $@;
+
+		    # don't close the connection entirely otherwise the receiving end
+		    # might not get all buffered data (and fails with 'connection reset by peer')
+		    shutdown($socket, 1);
+		} else {
+		    $match_volid_and_log->("[$target_sshinfo->{name}] $line");
+		}
+	    };
 
-	    # now close the socket
-	    close($socket);
-	    if (!close($info)) { # does waitpid()
-		die "import failed: $!\n" if $!;
-		die "import failed: exit code ".($?>>8)."\n";
-	    }
+	    eval {
+		run_command(
+		    $recv,
+		    outfunc => $handle_insecure_migration,
+		    errfunc => sub {
+			my $line = shift;
+
+			$match_volid_and_log->("[$target_sshinfo->{name}] $line");
+		    },
+		);
+	    };
+	    my $recv_err = $@;
+	    close($socket) if $socket;
+	    die "failed to run insecure migration: $recv_err\n" if $recv_err;
 
 	    die $send_error if $send_error;
 	} else {
-- 
2.39.2




^ permalink raw reply	[flat|nested] 3+ messages in thread

* [pve-devel] applied: [PATCH v3 storage] fix insecure migration failing if waiting on lock
  2024-04-17  9:48 [pve-devel] [PATCH v3 storage] fix insecure migration failing if waiting on lock Mira Limbeck
@ 2024-04-17 10:32 ` Thomas Lamprecht
  2024-04-17 10:32   ` Thomas Lamprecht
  0 siblings, 1 reply; 3+ messages in thread
From: Thomas Lamprecht @ 2024-04-17 10:32 UTC (permalink / raw)
  To: Proxmox VE development discussion, Mira Limbeck

Am 17/04/2024 um 11:48 schrieb Mira Limbeck:
> both STDOUT and STDERR are written into `$info` which is then parsed for
> IP and port of the target socket listening.
> when the ports file can't be locked immediately `trying to acquire
> lock...` is printed on STDERR and in turn written into `$info`.
> trying to parse the IP then fails, resulting in a migration or
> replication failing.
> 
> the bare open3 call is replaced by the run_command wrapper from
> pve-common to use a safe wrapper around open3 with the same
> functionality.
> STDERR is read separatey from STDOUT and the last line of STDERR is
> kept in case of errors.
> 
> Fixes: 57acd6a ("fix #1452: also log stderr of remote command with
> insecure storage migration")
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> v3:
>  - added log prefix for remote error logs
>  - fixed style issues
> 
> v2:
>  - incorporated Fiona's suggestions
>    - added `Fixes: ...` to commit message
>    - kept old ip/port matching including # untaint comments
>    - added logging for all messages in STDERR
>    - simplified branches
> 
>  src/PVE/Storage.pm | 91 ++++++++++++++++++++++++++++------------------
>  1 file changed, 55 insertions(+), 36 deletions(-)
> 
>

applied, thanks!


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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [pve-devel] applied: [PATCH v3 storage] fix insecure migration failing if waiting on lock
  2024-04-17 10:32 ` [pve-devel] applied: " Thomas Lamprecht
@ 2024-04-17 10:32   ` Thomas Lamprecht
  0 siblings, 0 replies; 3+ messages in thread
From: Thomas Lamprecht @ 2024-04-17 10:32 UTC (permalink / raw)
  To: Proxmox VE development discussion, Mira Limbeck

Am 17/04/2024 um 11:48 schrieb Mira Limbeck:
> both STDOUT and STDERR are written into `$info` which is then parsed for
> IP and port of the target socket listening.
> when the ports file can't be locked immediately `trying to acquire
> lock...` is printed on STDERR and in turn written into `$info`.
> trying to parse the IP then fails, resulting in a migration or
> replication failing.
> 
> the bare open3 call is replaced by the run_command wrapper from
> pve-common to use a safe wrapper around open3 with the same
> functionality.
> STDERR is read separatey from STDOUT and the last line of STDERR is
> kept in case of errors.
> 
> Fixes: 57acd6a ("fix #1452: also log stderr of remote command with
> insecure storage migration")
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> v3:
>  - added log prefix for remote error logs
>  - fixed style issues
> 
> v2:
>  - incorporated Fiona's suggestions
>    - added `Fixes: ...` to commit message
>    - kept old ip/port matching including # untaint comments
>    - added logging for all messages in STDERR
>    - simplified branches
> 
>  src/PVE/Storage.pm | 91 ++++++++++++++++++++++++++++------------------
>  1 file changed, 55 insertions(+), 36 deletions(-)
> 
>

applied, thanks!




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2024-04-17 10:32 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-17  9:48 [pve-devel] [PATCH v3 storage] fix insecure migration failing if waiting on lock Mira Limbeck
2024-04-17 10:32 ` [pve-devel] applied: " Thomas Lamprecht
2024-04-17 10:32   ` Thomas Lamprecht

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal