Libguestfs - Feb 2014 - [PATCH 1/2] mllib: add an hook to cleanup directories on exit

Much similar to unlink_on_exit, but recursively cleaning directories.
---
 mllib/common_utils.ml | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
index 3943417..f49ede6 100644
--- a/mllib/common_utils.ml
+++ b/mllib/common_utils.ml
@@ -386,6 +386,35 @@ let unlink_on_exit        registered_handlers := true
     )
 
+(* Remove a temporary directory on exit. *)
+let rmdir_on_exit +  let dirs = ref [] in
+  let registered_handlers = ref false in
+
+  let rec unlink_dirs () +    let rec recursive_rmdir fn +      if
Sys.is_directory fn then (
+        let names = Array.map (fun d -> fn // d) (Sys.readdir fn) in
+        Array.iter recursive_rmdir names;
+        Unix.rmdir fn
+      ) else
+        Unix.unlink fn
+    in
+    List.iter (
+      fun dir -> try recursive_rmdir dir with _ -> ()
+    ) !dirs
+  and register_handlers () +    (* Remove on exit. *)
+    at_exit unlink_dirs
+  in
+
+  fun dir ->
+    dirs := dir :: !dirs;
+    if not !registered_handlers then (
+      register_handlers ();
+      registered_handlers := true
+    )
+
 (* Using the libguestfs API, recursively remove only files from the
  * given directory.  Useful for cleaning /var/cache etc in sysprep
  * without removing the actual directory structure.  Also if 'dir' is
-- 
1.8.3.1

Create a temporary directory and tell gpg to use it as homedir, so
imported keys do not get into the user's keyring. This also avoid
importing the default key when a different one is needed to check the
signature.

The only exception is when a non-default fingerprint is used: in this
case, that key is read from the user's keyring, since it is where it is.
---
 builder/Makefile.am   |  5 +++++
 builder/mkdtemp-c.c   | 56 ++++++++++++++++++++++++++++++++++++++++++++++
 builder/mkdtemp.ml    | 19 ++++++++++++++++
 builder/mkdtemp.mli   | 20 +++++++++++++++++
 builder/sigchecker.ml | 61 ++++++++++++++++++++++++++++++++++++++++-----------
 po/POTFILES           |  1 +
 po/POTFILES-ml        |  1 +
 7 files changed, 150 insertions(+), 13 deletions(-)
 create mode 100644 builder/mkdtemp-c.c
 create mode 100644 builder/mkdtemp.ml
 create mode 100644 builder/mkdtemp.mli

diff --git a/builder/Makefile.am b/builder/Makefile.am
index 23336c0..d668377 100644
--- a/builder/Makefile.am
+++ b/builder/Makefile.am
@@ -48,6 +48,9 @@ SOURCES = \
 	index-parser-c.c \
 	list_entries.mli \
 	list_entries.ml \
+	mkdtemp.mli \
+	mkdtemp.ml \
+	mkdtemp-c.c \
 	paths.ml \
 	pxzcat.ml \
 	pxzcat.mli \
@@ -92,6 +95,8 @@ OBJECTS = \
 	pxzcat.cmx \
 	setlocale-c.o \
 	setlocale.cmx \
+	mkdtemp-c.o \
+	mkdtemp.cmx \
 	paths.cmx \
 	get_kernel.cmx \
 	downloader.cmx \
diff --git a/builder/mkdtemp-c.c b/builder/mkdtemp-c.c
new file mode 100644
index 0000000..2284e3a
--- /dev/null
+++ b/builder/mkdtemp-c.c
@@ -0,0 +1,56 @@
+/* virt-builder
+ * Copyright (C) 2014 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA.
+ */
+
+#include <config.h>
+
+#include <stdlib.h>
+#include <errno.h>
+#include <string.h>
+
+#include <caml/alloc.h>
+#include <caml/fail.h>
+#include <caml/memory.h>
+#include <caml/mlvalues.h>
+
+#ifdef HAVE_CAML_UNIXSUPPORT_H
+#include <caml/unixsupport.h>
+#else
+#define Nothing ((value) 0)
+extern void unix_error (int errcode, char * cmdname, value arg) Noreturn;
+#endif
+
+value
+virt_builder_mkdtemp (value val_pattern)
+{
+  CAMLparam1 (val_pattern);
+  CAMLlocal1 (rv);
+  char *pattern, *ret;
+
+  pattern = strdup (String_val (val_pattern));
+  if (pattern == NULL)
+    unix_error (errno, (char *) "strdup", val_pattern);
+
+  ret = mkdtemp (pattern);
+  if (ret == NULL)
+    unix_error (errno, (char *) "mkdtemp", val_pattern);
+
+  rv = caml_copy_string (ret);
+  free (pattern);
+
+  CAMLreturn (rv);
+}
diff --git a/builder/mkdtemp.ml b/builder/mkdtemp.ml
new file mode 100644
index 0000000..2e64862
--- /dev/null
+++ b/builder/mkdtemp.ml
@@ -0,0 +1,19 @@
+(* virt-builder
+ * Copyright (C) 2014 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *)
+
+external mkdtemp : string -> string = "virt_builder_mkdtemp"
diff --git a/builder/mkdtemp.mli b/builder/mkdtemp.mli
new file mode 100644
index 0000000..674e6f2
--- /dev/null
+++ b/builder/mkdtemp.mli
@@ -0,0 +1,20 @@
+(* virt-builder
+ * Copyright (C) 2014 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *)
+
+val mkdtemp : string -> string
+(** [mkdtemp pattern] Tiny wrapper to the C [mkdtemp]. *)
diff --git a/builder/sigchecker.ml b/builder/sigchecker.ml
index b20a186..10e342e 100644
--- a/builder/sigchecker.ml
+++ b/builder/sigchecker.ml
@@ -95,21 +95,38 @@ ZvXkQ3FVJwZoLmHw47vvlVpLD/4gi1SuHWieRvZ+UdDq00E348pm
 =neBW
 -----END PGP PUBLIC KEY BLOCK-----
 "
-let key_imported = ref false
 
 type t = {
   debug : bool;
   gpg : string;
   fingerprint : string;
   check_signature : bool;
+  gpghome : string;
+  mutable key_imported : bool;
 }
 
 let create ~debug ~gpg ~fingerprint ~check_signature +  (* Create a temporary
directory for gnupg. *)
+  let tmpdir = Mkdtemp.mkdtemp (Filename.temp_dir_name //
"vb.gpghome.XXXXXX") in
+  rmdir_on_exit tmpdir;
+  (* Run gpg once, so it can setup its own home directory, failing
+   * if it cannot.
+   *)
+  let cmd = sprintf "%s --homedir %s --list-keys%s"
+    gpg tmpdir (if debug then "" else " >/dev/null
2>&1") in
+  if debug then eprintf "%s\n%!" cmd;
+  let r = Sys.command cmd in
+  if r <> 0 then (
+    eprintf (f_"virt-builder: error: GPG failure: could not run GPG the
first time\nUse the '-v' option and look for earlier error
messages.\n");
+    exit 1
+  );
   {
     debug = debug;
     gpg = gpg;
     fingerprint = fingerprint;
     check_signature = check_signature;
+    gpghome = tmpdir;
+    key_imported = false;
   }
 
 (* Compare two strings of hex digits ignoring whitespace and case. *)
@@ -159,8 +176,9 @@ and do_verify t args    let status_file = Filename.temp_file
"vbstat" ".txt" in
   unlink_on_exit status_file;
   let cmd -    sprintf "%s --verify%s --status-file %s %s"
-        t.gpg (if t.debug then "" else " -q --logger-file
/dev/null")
+    sprintf "%s --homedir %s --verify%s --status-file %s %s"
+        t.gpg t.gpghome
+        (if t.debug then "" else " -q --logger-file
/dev/null")
         (quote status_file) args in
   if t.debug then eprintf "%s\n%!" cmd;
   let r = Sys.command cmd in
@@ -188,23 +206,40 @@ and do_verify t args      exit 1
   )
 
-(* Import the default public key. *)
+(* Import the requested public key. *)
 and import_key t -  if not !key_imported then (
-    let filename, chan = Filename.open_temp_file "vbpubkey"
".asc" in
-    unlink_on_exit filename;
-    output_string chan default_pubkey;
-    close_out chan;
-
-    let cmd = sprintf "%s --import %s%s"
-      t.gpg (quote filename)
+  if not t.key_imported then (
+    let keyfile = ref "" in
+    if equal_fingerprints default_fingerprint t.fingerprint then (
+      let filename, chan = Filename.open_temp_file "vbpubkey"
".asc" in
+      unlink_on_exit filename;
+      output_string chan default_pubkey;
+      close_out chan;
+      keyfile := filename
+    ) else (
+      let filename = Filename.temp_file "vbpubkey" ".asc"
in
+      unlink_on_exit filename;
+      let cmd = sprintf "%s --yes --armor --output %s --export %s%s"
+        t.gpg (quote filename) (quote t.fingerprint)
+        (if t.debug then "" else " >/dev/null
2>&1") in
+      if t.debug then eprintf "%s\n%!" cmd;
+      let r = Sys.command cmd in
+      if r <> 0 then (
+        eprintf (f_"virt-builder: error: could not export public key\nUse
the '-v' option and look for earlier error messages.\n");
+        exit 1
+      );
+      keyfile := filename
+    );
+
+    let cmd = sprintf "%s --homedir %s --import %s%s"
+      t.gpg t.gpghome (quote !keyfile)
       (if t.debug then "" else " >/dev/null
2>&1") in
     let r = Sys.command cmd in
     if r <> 0 then (
       eprintf (f_"virt-builder: error: could not import public key\nUse
the '-v' option and look for earlier error messages.\n");
       exit 1
     );
-    key_imported := true
+    t.key_imported <- true
   )
 
 type csum_t = SHA512 of string
diff --git a/po/POTFILES b/po/POTFILES
index c0d20f0..1de501e 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -4,6 +4,7 @@ builder/index-parser-c.c
 builder/index-scan.c
 builder/index-struct.c
 builder/index-validate.c
+builder/mkdtemp-c.c
 builder/pxzcat-c.c
 builder/setlocale-c.c
 cat/cat.c
diff --git a/po/POTFILES-ml b/po/POTFILES-ml
index 301fe81..56551ab 100644
--- a/po/POTFILES-ml
+++ b/po/POTFILES-ml
@@ -4,6 +4,7 @@ builder/downloader.ml
 builder/get_kernel.ml
 builder/index_parser.ml
 builder/list_entries.ml
+builder/mkdtemp.ml
 builder/paths.ml
 builder/pxzcat.ml
 builder/setlocale.ml
-- 
1.8.3.1

On Thu, Feb 20, 2014 at 11:53:16AM +0100, Pino Toscano
wrote:
> Much similar to unlink_on_exit, but recursively cleaning directories.
> ---
>  mllib/common_utils.ml | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
> index 3943417..f49ede6 100644
> --- a/mllib/common_utils.ml
> +++ b/mllib/common_utils.ml
> @@ -386,6 +386,35 @@ let unlink_on_exit >        registered_handlers :=
true
>      )
>  
> +(* Remove a temporary directory on exit. *)
> +let rmdir_on_exit > +  let dirs = ref [] in
> +  let registered_handlers = ref false in
> +
> +  let rec unlink_dirs () > +    let rec recursive_rmdir fn > +     
if Sys.is_directory fn then (
I suspect this will follow symlinks, so that if the temporary
directory contains a link like:

  /tmp/tmpdir/foo -> /

it will proceed to wipe out other bits of your filesystem.

An altogether easier, safety and faster way to do this is to do it
like supermin does:

https://github.com/libguestfs/supermin/blob/master/src/supermin_utils.ml#L91

Especially line 105 ff. but the rest of the function may be
interesting too in the context of patch 2/2.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Thursday 20 February 2014 13:08:40 Richard W.M. Jones
wrote:
> On Thu, Feb 20, 2014 at 11:53:16AM +0100, Pino Toscano wrote:
> > Much similar to unlink_on_exit, but recursively cleaning
> > directories.
> > ---
> > 
> >  mllib/common_utils.ml | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
> > index 3943417..f49ede6 100644
> > --- a/mllib/common_utils.ml
> > +++ b/mllib/common_utils.ml
> > @@ -386,6 +386,35 @@ let unlink_on_exit > > 
> >        registered_handlers := true
> >      
> >      )
> > 
> > +(* Remove a temporary directory on exit. *)
> > +let rmdir_on_exit > > +  let dirs = ref [] in
> > +  let registered_handlers = ref false in
> > +
> > +  let rec unlink_dirs () > > +    let rec recursive_rmdir fn
> > +      if Sys.is_directory fn then (
> 
> I suspect this will follow symlinks, so that if the temporary
> directory contains a link like:
> 
>   /tmp/tmpdir/foo -> /
> 
> it will proceed to wipe out other bits of your filesystem.
Uh duh, sorry for the oversight. Fixed it checking the file type using 
lstat.
> Especially line 105 ff. but the rest of the function may be
> interesting too in the context of patch 2/2.
Hm, it is kind of duplicating what mkdtemp already does, so I would 
rather keep using it. Pity OCaml does not offer it at all...

-- 
Pino Toscano

On Thu, Feb 20, 2014 at 11:53:17AM +0100, Pino Toscano
wrote:
> Create a temporary directory and tell gpg to use it as homedir, so
> imported keys do not get into the user's keyring. This also avoid
> importing the default key when a different one is needed to check the
> signature.
> 
> The only exception is when a non-default fingerprint is used: in this
> case, that key is read from the user's keyring, since it is where it
is.
The mkdtemp part is fine.  You could spin that off into a separate
commit, so it could be a candidate for backporting.

The rest I found a bit confusing.  What does it do exactly?
> diff --git a/builder/sigchecker.ml b/builder/sigchecker.ml
> index b20a186..10e342e 100644
> --- a/builder/sigchecker.ml
> +++ b/builder/sigchecker.ml
> @@ -95,21 +95,38 @@ ZvXkQ3FVJwZoLmHw47vvlVpLD/4gi1SuHWieRvZ+UdDq00E348pm
>  =neBW
>  -----END PGP PUBLIC KEY BLOCK-----
>  "
> -let key_imported = ref false
>  
>  type t = {
>    debug : bool;
>    gpg : string;
>    fingerprint : string;
>    check_signature : bool;
> +  gpghome : string;
> +  mutable key_imported : bool;
>  }
>  
>  let create ~debug ~gpg ~fingerprint ~check_signature > +  (* Create a
temporary directory for gnupg. *)
> +  let tmpdir = Mkdtemp.mkdtemp (Filename.temp_dir_name //
"vb.gpghome.XXXXXX") in
> +  rmdir_on_exit tmpdir;
> +  (* Run gpg once, so it can setup its own home directory, failing
> +   * if it cannot.
> +   *)
> +  let cmd = sprintf "%s --homedir %s --list-keys%s"
> +    gpg tmpdir (if debug then "" else " >/dev/null
2>&1") in
> +  if debug then eprintf "%s\n%!" cmd;
> +  let r = Sys.command cmd in
> +  if r <> 0 then (
> +    eprintf (f_"virt-builder: error: GPG failure: could not run GPG
the first time\nUse the '-v' option and look for earlier error
messages.\n");
> +    exit 1
> +  );
>    {
>      debug = debug;
>      gpg = gpg;
>      fingerprint = fingerprint;
>      check_signature = check_signature;
> +    gpghome = tmpdir;
> +    key_imported = false;
>    }
>  
>  (* Compare two strings of hex digits ignoring whitespace and case. *)
> @@ -159,8 +176,9 @@ and do_verify t args >    let status_file =
Filename.temp_file "vbstat" ".txt" in
>    unlink_on_exit status_file;
>    let cmd > -    sprintf "%s --verify%s --status-file %s %s"
> -        t.gpg (if t.debug then "" else " -q --logger-file
/dev/null")
> +    sprintf "%s --homedir %s --verify%s --status-file %s %s"
> +        t.gpg t.gpghome
> +        (if t.debug then "" else " -q --logger-file
/dev/null")
>          (quote status_file) args in
>    if t.debug then eprintf "%s\n%!" cmd;
>    let r = Sys.command cmd in
> @@ -188,23 +206,40 @@ and do_verify t args >      exit 1
>    )
>  
> -(* Import the default public key. *)
> +(* Import the requested public key. *)
>  and import_key t > -  if not !key_imported then (
> -    let filename, chan = Filename.open_temp_file "vbpubkey"
".asc" in
> -    unlink_on_exit filename;
> -    output_string chan default_pubkey;
> -    close_out chan;
> -
> -    let cmd = sprintf "%s --import %s%s"
> -      t.gpg (quote filename)
> +  if not t.key_imported then (
> +    let keyfile = ref "" in
> +    if equal_fingerprints default_fingerprint t.fingerprint then (
> +      let filename, chan = Filename.open_temp_file "vbpubkey"
".asc" in
> +      unlink_on_exit filename;
> +      output_string chan default_pubkey;
> +      close_out chan;
> +      keyfile := filename
> +    ) else (
> +      let filename = Filename.temp_file "vbpubkey"
".asc" in
> +      unlink_on_exit filename;
> +      let cmd = sprintf "%s --yes --armor --output %s --export
%s%s"
> +        t.gpg (quote filename) (quote t.fingerprint)
> +        (if t.debug then "" else " >/dev/null
2>&1") in
> +      if t.debug then eprintf "%s\n%!" cmd;
> +      let r = Sys.command cmd in
> +      if r <> 0 then (
> +        eprintf (f_"virt-builder: error: could not export public
key\nUse the '-v' option and look for earlier error messages.\n");
> +        exit 1
> +      );
> +      keyfile := filename
> +    );
> +
> +    let cmd = sprintf "%s --homedir %s --import %s%s"
> +      t.gpg t.gpghome (quote !keyfile)
>        (if t.debug then "" else " >/dev/null
2>&1") in
>      let r = Sys.command cmd in
>      if r <> 0 then (
>        eprintf (f_"virt-builder: error: could not import public
key\nUse the '-v' option and look for earlier error messages.\n");
>        exit 1
>      );
> -    key_imported := true
> +    t.key_imported <- true
>    )
>  
>  type csum_t = SHA512 of string
Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/