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

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

On 6/29/23 14:54, Richard W.M. Jones wrote:
> 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?
Ding ding ding, it does not :)

I'm happy you asked. (I was so frustrated to discover this *once again*
that I almost sent a separate tirade about it to the list! So I guess
now I'll use the opportunity...)

The short answer is that, if "Unix.chown" were out of the scope of the
"then", then it would also be out of the scope of "uid", and
so it
wouldn't compile. But the long answer is more interesting:

  # open Printf;;

  # if false then printf "1\n"; printf "2\n";;
  2
  - : unit = ()

And then compare:

  # if false then let () = () in printf "1\n"; printf
"2\n";;
  - : unit = ()

When we have "then" and a semiclon ";" but *no*
"let/in", then the
"then" binds more strongly than the semicolon.

When we have a "then", a "let/in", and a semicolon
";", then the
semicolon binds more strongly than the "in", *and* the
"let/in" binds
more strongly than the "then". Transitively, the semicolon binds more
strongly than the "then".

In other words, the "let/in" isn't just *inserted* between the
"then"
and the semicolon ";", preserving their relative binding strengths --
their relative binding strengths are *reversed*, without any explicit
parens!

This is *insane* in OCaml.

See also:

- the following article:
  http://ocamlverse.net/content/faq_if_semicolon.html

- the following video:
  https://www.youtube.com/watch?v=aj0bpOyv7Gs

In particular, the video claims at about 1:32 that the sequencing with
the semicolon

  e1; e2

is just syntactic sugar for

  let () = e1 in
  e2

Well, that claim is wrong. The purported equivalence holds only for

  ( e1; e2 )

(note the parens!), not for the naked

  e1; e2

Consider our original

  if false then printf "1\n"; printf "2\n"         [1]

versus

  if false then (printf "1\n"; printf "2\n")       [2]

The latter [2] *indeed* corresponds to:

  if false then
    let () = printf "1\n" in
    printf "2\n"

But what is the *former* [1], with the syntactic sugar removed? Well, it
turns out:

  let () = if false then printf "1\n" in
  printf "2\n"

Crazy.


In the patch, I removed the parens because they'd make the wrong
impression. They'd imply they made a difference relative to the default
bindings. But that's not the case. It's not the *presence* of a closing
paren *after* the Unix.chown call that matters, rather it's the
*absence* of a closing paren *before* Unix.chown -- because the latter
would change behavior:

let rec chown_for_libvirt_rhbz_1045069 file   let running_as_root = Unix.geteuid
() = 0 in
  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

(and this highlights how we'd be referencing "uid" out of scope).
> 
>>  (* 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.
> 
Laszlo