Libguestfs - Oct 2017 - Re: [PATCH] v2v: -i vmx: Refuse to load a disk image by accident.

If you accidentally point -i vmx at a disk image, it will try to load
the whole thing into memory and crash.  Catch this narrow case and
print an error.

However don't fail if ‘file’ is not installed or if we don't know what
the file is.
---
 v2v/parse_vmx.ml | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/v2v/parse_vmx.ml b/v2v/parse_vmx.ml
index 65d5a0edd..f6c34e2cf 100644
--- a/v2v/parse_vmx.ml
+++ b/v2v/parse_vmx.ml
@@ -268,6 +268,18 @@ let remove_vmx_escapes str  
 (* Parsing. *)
 let rec parse_file vmx_filename +  (* One person pointed -i vmx at the VMDK
file, resulting in an out
+   * of memory error.  Avoid this narrow case.
+   *)
+  let () +    let cmd = sprintf "file -b %s ||:" (quote vmx_filename)
in
+    let out = external_command cmd in
+    match out with
+    | line :: _ when String.find line "disk image" >= 0 ->
+       error (f_"-i vmx: %s: this may be a disk image.  You must specify
the .vmx file only on the command line.")
+             vmx_filename
+    | _ -> () in
+
   (* Read the whole file as a list of lines. *)
   let str = read_whole_file vmx_filename in
   if verbose () then eprintf "VMX file:\n%s\n" str;
-- 
2.13.2

On Wednesday, 11 October 2017 15:25:26 CEST Richard W.M. Jones
wrote:
> If you accidentally point -i vmx at a disk image, it will try to load
> the whole thing into memory and crash.  Catch this narrow case and
> print an error.
> 
> However don't fail if ‘file’ is not installed or if we don't know
what
> the file is.
> ---
>  v2v/parse_vmx.ml | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/v2v/parse_vmx.ml b/v2v/parse_vmx.ml
> index 65d5a0edd..f6c34e2cf 100644
> --- a/v2v/parse_vmx.ml
> +++ b/v2v/parse_vmx.ml
> @@ -268,6 +268,18 @@ let remove_vmx_escapes str >  
>  (* Parsing. *)
>  let rec parse_file vmx_filename > +  (* One person pointed -i vmx at
the VMDK file, resulting in an out
> +   * of memory error.  Avoid this narrow case.
> +   *)
> +  let () > +    let cmd = sprintf "file -b %s ||:" (quote
vmx_filename) in
> +    let out = external_command cmd in
> +    match out with
> +    | line :: _ when String.find line "disk image" >= 0 ->
> +       error (f_"-i vmx: %s: this may be a disk image.  You must
specify the .vmx file only on the command line.")
> +             vmx_filename
> +    | _ -> () in
> +
>    (* Read the whole file as a list of lines. *)
>    let str = read_whole_file vmx_filename in
>    if verbose () then eprintf "VMX file:\n%s\n" str;
Hm I'm not sure about this, since we don't do this for other parameters
(e.g. the XML for -i libvirtxml).

Anyway, if a check is deemed needed, my suggestion is to use what Cédric
wrote in its virt-builder-repository implementation:

+let get_mime_type filepath +  let file_cmd = "file --mime-type --brief
" ^ (quote filepath) in
+  match external_command file_cmd with
+  | [] -> None
+  | line :: _ -> Some line

... using Std_utils.which to check for the presence of `file`,
returning None if not available.

-- 
Pino Toscano

On Wed, Oct 11, 2017 at 05:17:14PM +0200, Pino Toscano
wrote:
> On Wednesday, 11 October 2017 15:25:26 CEST Richard W.M. Jones wrote:
> > If you accidentally point -i vmx at a disk image, it will try to load
> > the whole thing into memory and crash.  Catch this narrow case and
> > print an error.
> > 
> > However don't fail if ‘file’ is not installed or if we don't
know what
> > the file is.
> > ---
> >  v2v/parse_vmx.ml | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/v2v/parse_vmx.ml b/v2v/parse_vmx.ml
> > index 65d5a0edd..f6c34e2cf 100644
> > --- a/v2v/parse_vmx.ml
> > +++ b/v2v/parse_vmx.ml
> > @@ -268,6 +268,18 @@ let remove_vmx_escapes str > >  
> >  (* Parsing. *)
> >  let rec parse_file vmx_filename > > +  (* One person pointed -i
vmx at the VMDK file, resulting in an out
> > +   * of memory error.  Avoid this narrow case.
> > +   *)
> > +  let () > > +    let cmd = sprintf "file -b %s ||:"
(quote vmx_filename) in
> > +    let out = external_command cmd in
> > +    match out with
> > +    | line :: _ when String.find line "disk image" >= 0
->
> > +       error (f_"-i vmx: %s: this may be a disk image.  You must
specify the .vmx file only on the command line.")
> > +             vmx_filename
> > +    | _ -> () in
> > +
> >    (* Read the whole file as a list of lines. *)
> >    let str = read_whole_file vmx_filename in
> >    if verbose () then eprintf "VMX file:\n%s\n" str;
> 
> Hm I'm not sure about this, since we don't do this for other
parameters
> (e.g. the XML for -i libvirtxml).
Let's drop this for now, since it's user error.

What's probably needed is a more generic mechanism that we can use
where the tools call read_whole_file.

Rich.
> Anyway, if a check is deemed needed, my suggestion is to use what Cédric
> wrote in its virt-builder-repository implementation:
> 
> +let get_mime_type filepath > +  let file_cmd = "file --mime-type
--brief " ^ (quote filepath) in
> +  match external_command file_cmd with
> +  | [] -> None
> +  | line :: _ -> Some line
> 
> ... using Std_utils.which to check for the presence of `file`,
> returning None if not available.
> 
> -- 
> Pino Toscano

> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v