fix(runtime): ChildProcess::kill() doesn't require additional perms (#15339)

Fixes #15217.

cli/tests/integration/run_tests.rs

@@ -3435,6 +3435,11 @@ itest!(test_and_bench_are_noops_in_run {
   output_str: Some(""),
 });
 
+itest!(spawn_kill_permissions {
+  args: "run --quiet --unstable --allow-run=deno spawn_kill_permissions.ts",
+  output_str: Some(""),
+});
+
 itest!(followup_dyn_import_resolved {
   args: "run --unstable --allow-read run/followup_dyn_import_resolves/main.ts",
   output: "run/followup_dyn_import_resolves/main.ts.out",

cli/tests/testdata/spawn_kill_permissions.ts

@@ -0,0 +1,6 @@
+const child = new Deno.Command("deno", {
+  args: ["eval", "await new Promise(r => setTimeout(r, 2000))"],
+  stdout: "null",
+  stderr: "null",
+}).spawn();
+child.kill("SIGTERM");

cli/tests/unit/command_test.ts

@@ -867,3 +867,21 @@ Deno.test(
     }
   },
 );
+
+Deno.test(
+  { permissions: { run: true, read: true } },
+  async function commandKillAfterStatus() {
+    const command = new Deno.Command(Deno.execPath(), {
+      args: ["help"],
+      stdout: "null",
+      stderr: "null",
+    });
+    const child = command.spawn();
+    await child.status;
+    assertThrows(
+      () => child.kill(),
+      TypeError,
+      "Child process has already terminated.",
+    );
+  },
+);

runtime/js/40_process.js

@@ -200,6 +200,7 @@ function collectOutput(readableStream) {
 class ChildProcess {
   #rid;
   #waitPromiseId;
+  #waitComplete = false;
   #unrefed = false;
 
   #pid;
@@ -268,8 +269,8 @@ class ChildProcess {
     const waitPromise = core.opAsync("op_spawn_wait", this.#rid);
     this.#waitPromiseId = waitPromise[promiseIdSymbol];
     this.#status = PromisePrototypeThen(waitPromise, (res) => {
-      this.#rid = null;
       signal?.[abortSignal.remove](onAbort);
+      this.#waitComplete = true;
       return res;
     });
   }
@@ -317,10 +318,10 @@ class ChildProcess {
   }
 
   kill(signo = "SIGTERM") {
-    if (this.#rid === null) {
+    if (this.#waitComplete) {
       throw new TypeError("Child process has already terminated.");
     }
-    ops.op_kill(this.#pid, signo, "Deno.Child.kill()");
+    ops.op_spawn_kill(this.#rid, signo);
   }
 
   ref() {

runtime/ops/process.rs

@@ -2,6 +2,7 @@
 
 use super::check_unstable;
 use crate::permissions::PermissionsContainer;
+use deno_core::error::type_error;
 use deno_core::error::AnyError;
 use deno_core::op;
 use deno_core::serde_json;
@@ -106,6 +107,7 @@ deno_core::extension!(
     op_spawn_child,
     op_spawn_wait,
     op_spawn_sync,
+    op_spawn_kill,
     deprecated::op_run,
     deprecated::op_run_status,
     deprecated::op_kill,
@@ -115,7 +117,9 @@ deno_core::extension!(
   },
 );
 
-struct ChildResource(tokio::process::Child);
+/// Second member stores the pid separately from the RefCell. It's needed for
+/// `op_spawn_kill`, where the RefCell is borrowed mutably by `op_spawn_wait`.
+struct ChildResource(RefCell<tokio::process::Child>, u32);
 
 impl Resource for ChildResource {
   fn name(&self) -> Cow<str> {
@@ -302,7 +306,9 @@ fn spawn_child(
     .take()
     .map(|stderr| state.resource_table.add(ChildStderrResource::from(stderr)));
 
-  let child_rid = state.resource_table.add(ChildResource(child));
+  let child_rid = state
+    .resource_table
+    .add(ChildResource(RefCell::new(child), pid));
 
   Ok(Child {
     rid: child_rid,
@@ -328,17 +334,18 @@ async fn op_spawn_wait(
   state: Rc<RefCell<OpState>>,
   rid: ResourceId,
 ) -> Result<ChildStatus, AnyError> {
+  #![allow(clippy::await_holding_refcell_ref)]
   let resource = state
     .borrow_mut()
     .resource_table
-    .take::<ChildResource>(rid)?;
-  Rc::try_unwrap(resource)
-    .ok()
-    .unwrap()
-    .0
-    .wait()
-    .await?
-    .try_into()
+    .get::<ChildResource>(rid)?;
+  let result = resource.0.try_borrow_mut()?.wait().await?.try_into();
+  state
+    .borrow_mut()
+    .resource_table
+    .close(rid)
+    .expect("shouldn't have closed until now");
+  result
 }
 
 #[op]
@@ -366,6 +373,19 @@ fn op_spawn_sync(
   })
 }
 
+#[op]
+fn op_spawn_kill(
+  state: &mut OpState,
+  rid: ResourceId,
+  signal: String,
+) -> Result<(), AnyError> {
+  if let Ok(child_resource) = state.resource_table.get::<ChildResource>(rid) {
+    deprecated::kill(child_resource.1 as i32, &signal)?;
+    return Ok(());
+  }
+  Err(type_error("Child process has already terminated."))
+}
+
 mod deprecated {
   use super::*;