Re: [Libguestfs] [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(a)redhat.com&gt; >>> --- >>> >>> 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.