Laszlo Ersek
2023-Jun-29 16:40 UTC
[Libguestfs] [v2v PATCH v2 2/3] lib/utils: make "chown_for_libvirt_rhbz_1045069" fail hard
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. >
Richard W.M. Jones
2023-Jun-29 17:16 UTC
[Libguestfs] [v2v PATCH v2 2/3] lib/utils: make "chown_for_libvirt_rhbz_1045069" fail hard
On Thu, Jun 29, 2023 at 06:40:27PM +0200, Laszlo Ersek wrote:> 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?I think the parens are clearer. Don't need a v3, thanks! Rich.> 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. > >-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top