From d46419e5bf7d651fbd1b36f1305e2fc43cc55303 Mon Sep 17 00:00:00 2001 From: Nick Roberts Date: Tue, 2 Jul 2024 17:50:23 -0400 Subject: [PATCH 1/2] Add some more [sigprocmask] tests. This helps ensure the stability of existing behavior in light of the upcoming bugfix to `spawn_unix`. Signed-off-by: Nick Roberts --- test/tests.ml | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/test/tests.ml b/test/tests.ml index 117cdff..5f13b3a 100644 --- a/test/tests.ml +++ b/test/tests.ml @@ -171,7 +171,21 @@ let%test_unit "sigprocmask" = | _ -> failwith "unexpected" in run Sys.sigusr1; - run ~sigprocmask:(SIG_BLOCK, [ Sys.sigusr1 ]) Sys.sigkill) + run ~sigprocmask:(SIG_BLOCK, [ Sys.sigusr1 ]) Sys.sigkill; + let old_signals = Unix.sigprocmask SIG_BLOCK [Sys.sigusr1] in + (* The blocking of [sigusr1] is only propagated to the child process if the + sigprocmask is [SIG_BLOCK] or [SIG_UNBLOCK]. + *) + run Sys.sigusr1; + run ~sigprocmask:(SIG_BLOCK, []) Sys.sigkill; + run ~sigprocmask:(SIG_UNBLOCK, []) Sys.sigkill; + (* Unblocking sigusr1 in the child process. *) + run ~sigprocmask:(SIG_UNBLOCK, [Sys.sigusr1]) Sys.sigusr1; + run ~sigprocmask:(SIG_SETMASK, []) Sys.sigusr1; + (* Restore the old signal mask before finishing the test. *) + let (_ : int list) = Unix.sigprocmask SIG_SETMASK old_signals in + () + ) ;; (* This should be at the end to clean up the test environment *) From 5a63016a9bd296e7c834242482d7d3851e74e98e Mon Sep 17 00:00:00 2001 From: Nick Roberts Date: Tue, 2 Jul 2024 17:52:36 -0400 Subject: [PATCH 2/2] C stub bugfix in `spawn_unix`. Avoid traversing OCaml data structures when the runtime lock is not held. Signed-off-by: Nick Roberts --- src/spawn.ml | 7 ++++- src/spawn_stubs.c | 79 ++++++++++++++++++++++++++++++----------------- 2 files changed, 56 insertions(+), 30 deletions(-) diff --git a/src/spawn.ml b/src/spawn.ml index c2911e6..b07cab8 100644 --- a/src/spawn.ml +++ b/src/spawn.ml @@ -98,7 +98,7 @@ external spawn_unix -> stderr:Unix.file_descr -> use_vfork:bool -> setpgid:int option - -> sigprocmask:(Unix.sigprocmask_command * int list) option + -> sigprocmask:(Unix.sigprocmask_command * int array) option -> int = "spawn_unix_byte" "spawn_unix" @@ -179,6 +179,11 @@ let spawn | Vfork -> true | Fork -> false in + let sigprocmask = + match sigprocmask with + | Some (mask, signals) -> Some (mask, Array.of_list signals) + | None -> None + in backend ~env ~cwd ~prog ~argv ~stdin ~stdout ~stderr ~use_vfork ~setpgid ~sigprocmask ;; diff --git a/src/spawn_stubs.c b/src/spawn_stubs.c index c654b5c..63cfa87 100644 --- a/src/spawn_stubs.c +++ b/src/spawn_stubs.c @@ -399,6 +399,7 @@ CAMLprim value spawn_unix(value v_env, value v_sigprocmask) { CAMLparam4(v_env, v_cwd, v_prog, v_argv); + CAMLlocal2(v_sigprocmask_command, v_sigprocmask_signals); pid_t ret; struct spawn_info info; int result_pipe[2]; @@ -446,6 +447,30 @@ CAMLprim value spawn_unix(value v_env, Is_block(v_setpgid) ? Long_val(Field(v_setpgid, 0)) : 0; + enum caml_unix_sigprocmask_command sigprocmask_command; + int* sigprocmask_signals; /* array */ + mlsize_t sigprocmask_signals_length; + + if (!Is_block(v_sigprocmask)) { + sigprocmask_command = CAML_SIG_SETMASK; + sigprocmask_signals = NULL; + sigprocmask_signals_length = 0; + } else { + v_sigprocmask = Field(v_sigprocmask, 0); + + v_sigprocmask_command = Field(v_sigprocmask, 0); + v_sigprocmask_signals = Field(v_sigprocmask, 1); + + sigprocmask_command = Long_val(v_sigprocmask_command); + sigprocmask_signals_length = Wosize_val(v_sigprocmask_signals); + sigprocmask_signals = (int*)malloc(sizeof(int) * sigprocmask_signals_length); + + for (mlsize_t i = 0; i < sigprocmask_signals_length; i++) { + sigprocmask_signals[i] = + caml_convert_signal_number(Long_val(Field(v_sigprocmask_signals, i))); + } + } + caml_enter_blocking_section(); enter_safe_pipe_section(); @@ -455,6 +480,9 @@ CAMLprim value spawn_unix(value v_env, leave_safe_pipe_section(); caml_leave_blocking_section(); free_spawn_info(&info); + if (sigprocmask_signals != NULL) { + free(sigprocmask_signals); + } unix_error(error, "pipe", Nothing); } @@ -478,44 +506,34 @@ CAMLprim value spawn_unix(value v_env, sigfillset(&sigset); pthread_sigmask(SIG_SETMASK, &sigset, &saved_procmask); - if (v_sigprocmask == Val_long(0)) { - sigemptyset(&info.child_sigmask); - } else { - v_sigprocmask = Field(v_sigprocmask, 0); - value v_sigprocmask_command = Field(v_sigprocmask, 0); - enum caml_unix_sigprocmask_command sigprocmask_command = Long_val(v_sigprocmask_command); + switch (sigprocmask_command) { + case CAML_SIG_SETMASK: + sigemptyset(&info.child_sigmask); + break; + + case CAML_SIG_BLOCK: + case CAML_SIG_UNBLOCK: + info.child_sigmask = saved_procmask; + break; + default: + caml_failwith("Unknown sigprocmask action"); + } + + for (mlsize_t i = 0; i < sigprocmask_signals_length; i++) { + int signal = sigprocmask_signals[i]; switch (sigprocmask_command) { case CAML_SIG_SETMASK: - sigemptyset(&info.child_sigmask); + case CAML_SIG_BLOCK: + sigaddset(&info.child_sigmask, signal); break; - case CAML_SIG_BLOCK: case CAML_SIG_UNBLOCK: - info.child_sigmask = saved_procmask; + sigdelset(&info.child_sigmask, signal); break; default: - caml_failwith("Unknown sigprocmask action"); - } - - value v_signals_list = Field(v_sigprocmask, 1); - for (; v_signals_list != Val_emptylist; - v_signals_list = Field(v_signals_list, 1)) { - int signal = caml_convert_signal_number(Long_val(Field(v_signals_list, 0))); - switch (sigprocmask_command) { - case CAML_SIG_SETMASK: - case CAML_SIG_BLOCK: - sigaddset(&info.child_sigmask, signal); - break; - - case CAML_SIG_UNBLOCK: - sigdelset(&info.child_sigmask, signal); - break; - - default: - assert(0); - } + assert(0); } } @@ -529,6 +547,9 @@ CAMLprim value spawn_unix(value v_env, leave_safe_pipe_section(); free_spawn_info(&info); + if (sigprocmask_signals != NULL) { + free(sigprocmask_signals); + } close(result_pipe[1]); got_error = 0;