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

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

On Thu, Jun 29, 2023 at 05:39:34PM +0200, Laszlo Ersek
wrote:
> 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. 
Well, unless there happened to be a "uid" symbol somewhere in the
outer scope (or we added one later), or in any "open"-d module.

Tirade aside, not having the parentheses makes this code fragile.
eg. It'll break unpredictably if someone inserts a statement after the
debug command.
> 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.
That's a bit surprising, and I've been programming in ML for about
30 years.  However just like in C (which has another variant of the
same issue) putting in brackets helps to clarify what you really mean.
> 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
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