Libguestfs - Jun 2023 - [v2v PATCH v2 2/3] lib/utils: make "chown_for_libvirt_rhbz_1045069" fail hard

Currently "chown_for_libvirt_rhbz_1045069" is best effort; if it
fails, we
suppress the exception (we log it in verbose mode only, even).

That's not proved helpful: it almost certainly leads to later errors, but
those errors are less clear than the original (suppressed) exception.
Namely, the user sees something like
> Failed to connect to '/tmp/v2v.sKlulY/in0': Permission denied
rather than
> Failed to connect socket to '/var/run/libvirt/virtqemud-sock-ro':
> Connection refused
So just allow the exception to propagate outwards.

And then, now that "chown_for_libvirt_rhbz_1045069" will be able to
fail,
hoist the call to "On_exit.rm_rf" before the call to
"chown_for_libvirt_rhbz_1045069", after creating the v2v temporary
directory. In the current order, if "chown_for_libvirt_rhbz_1045069"
threw
an exception, then we'd leak the temp dir in the filesystem.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2182024
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    v2:
    
    - new patch

 lib/utils.mli |  5 +---
 lib/utils.ml  | 26 ++++++++------------
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/lib/utils.mli b/lib/utils.mli
index cf88a467fd54..391a2a351ec7 100644
--- a/lib/utils.mli
+++ b/lib/utils.mli
@@ -67,10 +67,7 @@ val chown_for_libvirt_rhbz_1045069 : string -> unit
     to root-owned files and directories.  To fix this, provide
     a function to chown things we might need to qemu:root so
     qemu can access them.  Note that root normally ignores
-    permissions so can still access the resource.
-
-    This is best-effort.  If something fails then we carry
-    on and hope for the best. *)
+    permissions so can still access the resource. *)
 
 val error_if_no_ssh_agent : unit -> unit
 
diff --git a/lib/utils.ml b/lib/utils.ml
index 174c01b1e92f..7d69c9e0d177 100644
--- a/lib/utils.ml
+++ b/lib/utils.ml
@@ -149,21 +149,15 @@ let backend_is_libvirt ()  
 let rec chown_for_libvirt_rhbz_1045069 file    let running_as_root =
Unix.geteuid () = 0 in
-  if running_as_root && backend_is_libvirt () then (
-    try
-      let user = Option.value ~default:"qemu" (libvirt_qemu_user ())
in
-      let uid -        if String.is_prefix user "+" then
-          int_of_string (String.sub user 1 (String.length user - 1))
-        else
-          (Unix.getpwnam user).pw_uid in
-      debug "setting owner of %s to %d:root" file uid;
-      Unix.chown file uid 0
-    with
-    | exn -> (* Print exception, but continue. *)
-       debug "could not set owner of %s: %s"
-         file (Printexc.to_string exn)
-  )
+  if running_as_root && backend_is_libvirt () then
+    let user = Option.value ~default:"qemu" (libvirt_qemu_user ()) in
+    let uid +      if String.is_prefix user "+" then
+        int_of_string (String.sub user 1 (String.length user - 1))
+      else
+        (Unix.getpwnam user).pw_uid in
+    debug "setting owner of %s to %d:root" file uid;
+    Unix.chown file uid 0
 
 (* Get the local user that libvirt uses to run qemu when we are
  * running as root.  This is returned as an optional string
@@ -205,8 +199,8 @@ let error_if_no_ssh_agent ()  (* Create the directory
containing inX and outX sockets. *)
 let create_v2v_directory ()    let d = Mkdtemp.temp_dir "v2v." in
-  chown_for_libvirt_rhbz_1045069 d;
   On_exit.rm_rf d;
+  chown_for_libvirt_rhbz_1045069 d;
   d
 
 (* Wait for a file to appear until a timeout. *)

On Thu, Jun 29, 2023 at 02:34:42PM +0200, Laszlo Ersek
wrote:
> Currently "chown_for_libvirt_rhbz_1045069" is best effort; if it
fails, we
> suppress the exception (we log it in verbose mode only, even).
> 
> That's not proved helpful: it almost certainly leads to later errors,
but
> those errors are less clear than the original (suppressed) exception.
> Namely, the user sees something like
> 
> > Failed to connect to '/tmp/v2v.sKlulY/in0': Permission denied
> 
> rather than
> 
> > Failed to connect socket to
'/var/run/libvirt/virtqemud-sock-ro':
> > Connection refused
> 
> So just allow the exception to propagate outwards.
> 
> And then, now that "chown_for_libvirt_rhbz_1045069" will be able
to fail,
> hoist the call to "On_exit.rm_rf" before the call to
> "chown_for_libvirt_rhbz_1045069", after creating the v2v
temporary
> directory. In the current order, if
"chown_for_libvirt_rhbz_1045069" threw
> an exception, then we'd leak the temp dir in the filesystem.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2182024
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
> 
> Notes:
>     v2:
>     
>     - new patch
> 
>  lib/utils.mli |  5 +---
>  lib/utils.ml  | 26 ++++++++------------
>  2 files changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/utils.mli b/lib/utils.mli
> index cf88a467fd54..391a2a351ec7 100644
> --- a/lib/utils.mli
> +++ b/lib/utils.mli
> @@ -67,10 +67,7 @@ val chown_for_libvirt_rhbz_1045069 : string -> unit
>      to root-owned files and directories.  To fix this, provide
>      a function to chown things we might need to qemu:root so
>      qemu can access them.  Note that root normally ignores
> -    permissions so can still access the resource.
> -
> -    This is best-effort.  If something fails then we carry
> -    on and hope for the best. *)
> +    permissions so can still access the resource. *)
>  
>  val error_if_no_ssh_agent : unit -> unit
>  
> diff --git a/lib/utils.ml b/lib/utils.ml
> index 174c01b1e92f..7d69c9e0d177 100644
> --- a/lib/utils.ml
> +++ b/lib/utils.ml
> @@ -149,21 +149,15 @@ let backend_is_libvirt () >  
>  let rec chown_for_libvirt_rhbz_1045069 file >    let running_as_root =
Unix.geteuid () = 0 in
> -  if running_as_root && backend_is_libvirt () then (
> -    try
> -      let user = Option.value ~default:"qemu" (libvirt_qemu_user
()) in
> -      let uid > -        if String.is_prefix user "+" then
> -          int_of_string (String.sub user 1 (String.length user - 1))
> -        else
> -          (Unix.getpwnam user).pw_uid in
> -      debug "setting owner of %s to %d:root" file uid;
> -      Unix.chown file uid 0
> -    with
> -    | exn -> (* Print exception, but continue. *)
> -       debug "could not set owner of %s: %s"
> -         file (Printexc.to_string exn)
> -  )
> +  if running_as_root && backend_is_libvirt () then
> +    let user = Option.value ~default:"qemu" (libvirt_qemu_user
()) in
> +    let uid > +      if String.is_prefix user "+" then
> +        int_of_string (String.sub user 1 (String.length user - 1))
> +      else
> +        (Unix.getpwnam user).pw_uid in
> +    debug "setting owner of %s to %d:root" file uid;
> +    Unix.chown file uid 0
Hmm, doesn't this need parens around the 'then' clause?
>  (* Get the local user that libvirt uses to run qemu when we are
>   * running as root.  This is returned as an optional string
> @@ -205,8 +199,8 @@ let error_if_no_ssh_agent () >  (* Create the
directory containing inX and outX sockets. *)
>  let create_v2v_directory () >    let d = Mkdtemp.temp_dir
"v2v." in
> -  chown_for_libvirt_rhbz_1045069 d;
>    On_exit.rm_rf d;
> +  chown_for_libvirt_rhbz_1045069 d;
>    d
Yes, as you explain in the commit message this hunk is needed (and
dependent) because the chown might now fail.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html