From 40ea41fcd935de652b1afaf8ca49f9281a51af31 Mon Sep 17 00:00:00 2001 From: Etienne Millon Date: Thu, 14 Mar 2024 14:09:50 +0100 Subject: [PATCH] fix(codesign): run hook for promoted executables (#10263) * fix(codesign): run hook for promoted executables Fixes #9272 The fix in #8361 relies on a `~executable` argument that was not passed in the promotion case. Instead, we call `stat` in `Conf.run_sign_hook`. Signed-off-by: Etienne Millon --- bin/install_uninstall.ml | 2 +- doc/changes/10263.md | 2 ++ src/dune_rules/artifact_substitution.ml | 32 +++++++++------------ src/dune_rules/artifact_substitution.mli | 1 - test/blackbox-tests/test-cases/dune | 8 ------ test/blackbox-tests/test-cases/github9272.t | 24 +--------------- 6 files changed, 18 insertions(+), 51 deletions(-) create mode 100644 doc/changes/10263.md diff --git a/bin/install_uninstall.ml b/bin/install_uninstall.ml index 8c181f52b4f..ae1af3444e6 100644 --- a/bin/install_uninstall.ml +++ b/bin/install_uninstall.ml @@ -342,7 +342,7 @@ module File_ops_real (W : sig = let chmod = if executable then fun _ -> 0o755 else fun _ -> 0o644 in match (special_file : Special_file.t option) with - | None -> Artifact_substitution.copy_file ~conf ~executable ~src ~dst ~chmod () + | None -> Artifact_substitution.copy_file ~conf ~src ~dst ~chmod () | Some sf -> (* CR-rgrinberg: slow copying *) let ic, oc = Io.setup_copy ~chmod ~src ~dst () in diff --git a/doc/changes/10263.md b/doc/changes/10263.md new file mode 100644 index 00000000000..daa901aed2c --- /dev/null +++ b/doc/changes/10263.md @@ -0,0 +1,2 @@ +- regression fix: sign executables that are promoted into the source tree + (#10263, fixes #9272, @emillon) diff --git a/src/dune_rules/artifact_substitution.ml b/src/dune_rules/artifact_substitution.ml index 36c7f75f13e..07aaac66544 100644 --- a/src/dune_rules/artifact_substitution.ml +++ b/src/dune_rules/artifact_substitution.ml @@ -175,10 +175,18 @@ module Conf = struct match has_subst with | No_substitution -> Fiber.return () | Some_substitution -> - Memo.run t.sign_hook - |> Fiber.bind ~f:(function - | Some hook -> hook file - | None -> Fiber.return ()) + let executable = + match Path.Untracked.stat file with + | Error _ -> false + | Ok { st_perm; _ } -> Path.Permissions.test Path.Permissions.execute st_perm + in + if executable + then + Memo.run t.sign_hook + |> Fiber.bind ~f:(function + | Some hook -> hook file + | None -> Fiber.return ()) + else Fiber.return () ;; end @@ -675,15 +683,7 @@ let replace_if_different ~delete_dst_if_it_is_a_directory ~src ~dst = if not up_to_date then Path.rename src dst ;; -let copy_file - ~conf - ?(executable = false) - ?chmod - ?(delete_dst_if_it_is_a_directory = false) - ~src - ~dst - () - = +let copy_file ~conf ?chmod ?(delete_dst_if_it_is_a_directory = false) ~src ~dst () = (* We create a temporary file in the same directory to ensure it's on the same partition as [dst] (otherwise, [Path.rename temp_file dst] won't work). The prefix ".#" is used because Dune ignores such files and so creating this @@ -698,11 +698,7 @@ let copy_file let open Fiber.O in Path.parent dst |> Option.iter ~f:Path.mkdir_p; let* has_subst = copy_file_non_atomic ~conf ?chmod ~src ~dst:temp_file () in - let+ () = - if executable - then Conf.run_sign_hook conf ~has_subst temp_file - else Fiber.return () - in + let+ () = Conf.run_sign_hook conf ~has_subst temp_file in replace_if_different ~delete_dst_if_it_is_a_directory ~src:temp_file ~dst) ~finally:(fun () -> Path.unlink_no_err temp_file; diff --git a/src/dune_rules/artifact_substitution.mli b/src/dune_rules/artifact_substitution.mli index ba85c40ec4f..b341c0fef61 100644 --- a/src/dune_rules/artifact_substitution.mli +++ b/src/dune_rules/artifact_substitution.mli @@ -53,7 +53,6 @@ val decode : string -> t option and then atomically renamed to [dst]. *) val copy_file : conf:Conf.t - -> ?executable:bool -> ?chmod:(int -> int) -> ?delete_dst_if_it_is_a_directory:bool -> src:Path.t diff --git a/test/blackbox-tests/test-cases/dune b/test/blackbox-tests/test-cases/dune index 2db6f0a393b..5457f0c02ea 100644 --- a/test/blackbox-tests/test-cases/dune +++ b/test/blackbox-tests/test-cases/dune @@ -138,11 +138,3 @@ (enabled_if (not %{arch_sixtyfour})) (deps %{bin:ocaml})) - -(cram - (applies_to github9272) - (enabled_if - (and - (= %{system} macosx) - (= %{architecture} arm64))) - (deps %{bin:ocaml})) diff --git a/test/blackbox-tests/test-cases/github9272.t b/test/blackbox-tests/test-cases/github9272.t index f8ebdf440c2..6ef06ed12ac 100644 --- a/test/blackbox-tests/test-cases/github9272.t +++ b/test/blackbox-tests/test-cases/github9272.t @@ -17,26 +17,4 @@ the source tree, the executable in the source tree segfaults. $ dune build -Test peculiarity: we can not do ./hello.exe directly, because the shell itself -(that runs the cram test) will display the pid of the crashing process. -Instead, we start and wait for it in an OCaml program and only display the -code. - -Once the bug is fixed, this can be replaced by just `./hello.exe` and the test -can be enabled for all systems. - - $ cat > exec.ml << EOF - > let () = - > let pid = - > Unix.create_process - > "./hello.exe" [|"./hello.exe"|] - > Unix.stdin Unix.stdout Unix.stderr - > in - > match Unix.waitpid [] pid with - > | _, WEXITED n -> Printf.printf "WEXITED %d" n - > | _, WSTOPPED n -> Printf.printf "WSTOPPED %d" n - > | _, WSIGNALED n -> Printf.printf "WSIGNALED %d" n - > EOF - - $ ocaml -I +unix unix.cma exec.ml - WSIGNALED -7 + $ ./hello.exe