[Libguestfs] [v2v PATCH v2 2/3] lib/utils: make "chown_for_libvirt_rhbz_1045069" fail hard
Richard W.M. Jones
rjones at redhat.com
Thu Jun 29 15:48:09 UTC 2023
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
More information about the Libguestfs
mailing list