[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
On Wednesday, 11 October 2017 18:59:39 CEST Richard W.M. Jones wrote:
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.
An option here could be adding an optional parameter to read_whole_file, i.e. a int64 for the maximum size allowed, and error out when the requested file is bigger than that. Then limiting to, say, 1MB the size of files in -i vmx, and -i libvirtxml should do the job. -- Pino Toscano
participants (2)
-
Pino Toscano -
Richard W.M. Jones