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

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

On 6/29/23 17:48, Richard W.M. Jones wrote:
> 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.
Inserting another statement between the debug and the Unix.chown will
not change a thing; they are all gobbled up by the innermost "in" just
the same.

There *is* some fragility indeed, but in the opposite direction -- if we
add something after the "Unix.chown", and we don't want it to
depend on
the "if", then we certainly need the parens.

Either way, I can restore the parens. Do you want me to submit a v3?

Laszlo
> 
>> 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.
>