[pve-devel] [RFC container] Create: fix architecture detection in restore_archive

[Wolfgang Bumiller]

First a general remark about dying in forks:
Since it is a fork it will use the same path out as the parent process
would, doing the same kind of exception handling etc. This means it'll
end up doing the same cleanup. Eg. when creating a container fails to
use this function, the child process already starts deleting the
container's disk images, then the parent tries to do the same.

The next issue is that - since you're using the return code to
communicate the elf class - you hide the fact that you "know" that the
return code you get after die() is an invalid one.

Instead, after the main eval block in the child process, use `warn $@`
and explicitly call POSIX::_exit(0); (in this case we know that 0 is an
invalid elf class.)
Contrary to exit() this will not call any END{} blocks or DESTROY()
methods for existing perl variables.

On Thu, Dec 15, 2016 at 11:39:31AM +0100, Thomas Lamprecht wrote:
> For detecting a CT templates architecture we used the `file -b -L`
> output from the PVE host side.
> If the container has a link:
> /bin/sh -> /bin/bash
> (Alpine Linux does that) the '-L' flag from file resolves the
> $rootfs/bin/sh to /bin/bash and thus checks the architecture of
> bash on the PVE system, which is always 64 bit.
> 
> Add a helper which chroots in the rootfs and opens /bin/sh to read
> the first 5 bytes, 4 bytes for the ELF magic number and the fifth for
> the ELF class, which tells us if we have a 32 or 64 bit ELF binary.
> 
> Return this information as an exit code to the parent.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> ---
> 
> This approach was chosen as manually resolving links with all its edge cases
> is a lot of work, and with this we can scrap the fun_command of `file -b -L`
> altogether
> 
> The get_elf_class helper may be a candidate for pve-common (PVE::Tools ?)
> but as we do not use this functionality else where, AFAIK, I kept it here
> for now
> 
> 
>  src/PVE/LXC/Create.pm | 68 +++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 53 insertions(+), 15 deletions(-)
> 
> diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
> index 11bc00d..e2c58b8 100644
> --- a/src/PVE/LXC/Create.pm
> +++ b/src/PVE/LXC/Create.pm
> @@ -23,6 +23,47 @@ sub next_free_nbd_dev {
>      die "unable to find free nbd device\n";
>  }
>  
> +sub get_elf_class {
> +    my ($rootfs, $binary_fn) = @_;
> +
> +    my $child = fork();
> +
> +    if (!defined($child)) {
> +	die "get_elf_class - fork failed: $!\n";
> +    } elsif ($child == 0) {
> +	eval {
> +	    chroot $rootfs;

Misses a check for errors.

> +
> +	    my $fh;
> +	    open ($fh, "<", $binary_fn) or die "'$binary_fn' not found in '$rootfs'!\n";

Don't "assume" you get ENOENT, just use $!, like:
  die "open($binary_fn): $!\n"

> +	    binmode($fh);
> +
> +	    my $data;
> +	    read($fh, $data, 5); # 4 bytes ELF magic number and 1 byte ELF class

Misses an error check. eg.:
    my $got = read($fh, $data, 5);
    die "read error: $!\n" if !defined($got); #error case
    POSIX::_exit(0) if $got < 5; # valid but short read, so not an ELF

> +
> +	    my ($magic, $class) = unpack("A4C", $data);
> +
> +	    die "'$binary_fn' does not resolve to an ELF!\n" if $magic ne "\177ELF";
> +
> +	    die "'$binary_fn' has unknown ELF class '$class'!\n"
> +		if ($class != 1 && $class != 2);
> +
> +	    exit ($class) # tell parent the elf class

Consider using POSIX::_exit().

> +	};
> +	die $@; # use eval/die else the exit code can be changed by read or open
> +    }
> +
> +    waitpid($child, 0);
> +    my $exit_code = $?;
> +    if (my $sig = ($exit_code & 127)) {
> +	die "got signal $sig";
> +    } else {
> +	$exit_code = ($exit_code >> 8);
> +    }
> +
> +    return $exit_code;
> +}
> +
>  sub restore_archive {
>      my ($archive, $rootdir, $conf, $no_unpack_error) = @_;
>  
> @@ -53,21 +94,18 @@ sub restore_archive {
>      # if arch is set, we do not try to autodetect it
>      return if defined($conf->{arch});
>  
> -    # determine file type of /usr/bin/file itself to get guests' architecture
> -    $cmd = [@$userns_cmd, '/usr/bin/file', '-b', '-L', "$rootdir/bin/sh"];
> -    PVE::Tools::run_command($cmd, outfunc => sub {
> -	shift =~ /^ELF (\d{2}-bit)/; # safely assumes x86 linux
> -	my $arch_str = $1;
> -	$conf->{'arch'} = 'amd64'; # defaults to 64bit
> -	if(defined($arch_str)) {
> -	    $conf->{'arch'} = 'i386' if $arch_str =~ /32/;
> -	    print "Detected container architecture: $conf->{'arch'}\n";
> -	} else {
> -	    print "CT architecture detection failed, falling back to amd64.\n" .
> -	          "Edit the config in /etc/pve/nodes/{node}/lxc/{vmid}.conf " .
> -	          "to set another architecture.\n";
> -	}
> -    });
> +
> +    my $elf_class = get_elf_class($rootdir, '/bin/sh'); # /bin/sh is POSIX mandatory
> +
> +    $conf->{'arch'} = 'amd64'; # defaults to 64bit
> +    if ($elf_class == 1 || $elf_class == 2) {
> +	$conf->{'arch'} = 'i386' if $elf_class == 1;
> +	print "Detected container architecture: $conf->{'arch'}\n";
> +    } else {
> +	print "CT architecture detection failed, falling back to amd64.\n" .
> +	      "Edit the config in /etc/pve/nodes/{node}/lxc/{vmid}.conf " .
> +	      "to set another architecture.\n";
> +    }
>  }
>  
>  sub recover_config {
> -- 
> 2.1.4