fix: validate integer values in Deno.exitCode setter (denoland#24068)

crowlKats · Jun 3, 2024 · eda43c4 · eda43c4
1 parent 754f21f
commit eda43c4
Show file tree

Hide file tree

Showing 5 changed files with 90 additions and 135 deletions.
diff --git a/cli/tests/unit/os_test.ts b/cli/tests/unit/os_test.ts
diff --git a/ext/node/polyfills/process.ts b/ext/node/polyfills/process.ts
@@ -19,6 +19,7 @@ import { report } from "ext:deno_node/internal/process/report.ts";
 import { validateString } from "ext:deno_node/internal/validators.mjs";
 import {
  ERR_INVALID_ARG_TYPE,
+ ERR_OUT_OF_RANGE,
  ERR_UNKNOWN_SIGNAL,
  errnoException,
 } from "ext:deno_node/internal/errors.ts";
@@ -439,38 +440,22 @@ Object.defineProperty(Process.prototype, "exitCode", {
  return ProcessExitCode;
  },
  set(code: number | string | null | undefined) {
- let parsedCode;
-
- if (typeof code === "number") {
- if (Number.isNaN(code)) {
- parsedCode = 1;
- denoOs.setExitCode(parsedCode);
- ProcessExitCode = parsedCode;
- return;
- }
-
- // This is looser than `denoOs.setExitCode` which requires exit code
- // to be decimal or string of a decimal, but Node accept eg. 0x10.
- parsedCode = parseInt(code);
- denoOs.setExitCode(parsedCode);
- ProcessExitCode = parsedCode;
- return;
+ let parsedCode: number;
+ if (code == null) {
+ parsedCode = 0;
+ } else if (typeof code === "number") {
+ parsedCode = code;
+ } else if (typeof code === "string") {
+ parsedCode = Number(code);
+ } else {
+ throw new ERR_INVALID_ARG_TYPE("code", "number", code);
  }
 
- if (typeof code === "string") {
- parsedCode = parseInt(code);
- if (Number.isNaN(parsedCode)) {
- throw new TypeError(
- `The "code" argument must be of type number. Received type ${typeof code} (${code})`,
- );
- }
- denoOs.setExitCode(parsedCode);
- ProcessExitCode = parsedCode;
- return;
+ if (!Number.isInteger(parsedCode)) {
+ throw new ERR_OUT_OF_RANGE("code", "an integer", parsedCode);
  }
 
- // TODO(bartlomieju): hope for the best here. This should be further tightened.
- denoOs.setExitCode(code);
+ denoOs.setExitCode(parsedCode);
  ProcessExitCode = code;
  },
 });

diff --git a/runtime/js/30_os.js b/runtime/js/30_os.js
@@ -22,7 +22,8 @@ import {
 const {
  Error,
  FunctionPrototypeBind,
- NumberParseInt,
+ NumberIsInteger,
+ RangeError,
  SymbolFor,
  TypeError,
 } = primordials;
@@ -102,13 +103,17 @@ function getExitCode() {
 }
 
 function setExitCode(value) {
- const code = NumberParseInt(value, 10);
- if (typeof code !== "number") {
+ if (typeof value !== "number") {
  throw new TypeError(
- `Exit code must be a number, got: ${code} (${typeof code}).`,
+ `Exit code must be a number, got: ${value} (${typeof value})`,
  );
  }
- op_set_exit_code(code);
+ if (!NumberIsInteger(value)) {
+ throw new RangeError(
+ `Exit code must be an integer, got: ${value}`,
+ );
+ }
+ op_set_exit_code(value);
 }
 
 function setEnv(key, value) {

diff --git a/tests/unit/os_test.ts b/tests/unit/os_test.ts
@@ -302,3 +302,70 @@ Deno.test(function memoryUsage() {
  assert(typeof mem.external === "number");
  assert(mem.rss >= mem.heapTotal);
 });
+
+Deno.test("Deno.exitCode getter and setter", () => {
+ // Initial value is 0
+ assertEquals(Deno.exitCode, 0);
+
+ try {
+ // Set a new value
+ Deno.exitCode = 5;
+ assertEquals(Deno.exitCode, 5);
+ } finally {
+ // Reset to initial value
+ Deno.exitCode = 0;
+ }
+
+ assertEquals(Deno.exitCode, 0);
+});
+
+Deno.test("Setting Deno.exitCode to non-number throws TypeError", () => {
+ // Throws on non-number values
+ assertThrows(
+ () => {
+ // @ts-expect-error Testing for runtime error
+ Deno.exitCode = "123";
+ },
+ TypeError,
+ "Exit code must be a number, got: 123 (string)",
+ );
+
+ // Throws on bigint values
+ assertThrows(
+ () => {
+ // @ts-expect-error Testing for runtime error
+ Deno.exitCode = 1n;
+ },
+ TypeError,
+ "Exit code must be a number, got: 1 (bigint)",
+ );
+});
+
+Deno.test("Setting Deno.exitCode to non-integer throws RangeError", () => {
+ // Throws on non-integer values
+ assertThrows(
+ () => {
+ Deno.exitCode = 3.14;
+ },
+ RangeError,
+ "Exit code must be an integer, got: 3.14",
+ );
+});
+
+Deno.test("Setting Deno.exitCode does not cause an immediate exit", () => {
+ let exited = false;
+
+ const originalExit = Deno.exit;
+ // @ts-expect-error; read-only
+ Deno.exit = () => {
+ exited = true;
+ };
+
+ try {
+ Deno.exitCode = 1;
+ assertEquals(exited, false);
+ } finally {
+ Deno.exit = originalExit;
+ Deno.exitCode = 0;
+ }
+});
diff --git a/tests/unit_node/process_test.ts b/tests/unit_node/process_test.ts
@@ -813,10 +813,6 @@ Deno.test("process.exitCode in should change exit code", async () => {
  "import process from 'node:process'; process.exitCode = 127;",
  127,
  );
- await exitCodeTest(
- "import process from 'node:process'; process.exitCode = 2.5;",
- 2,
- );
  await exitCodeTest(
  "import process from 'node:process'; process.exitCode = '10';",
  10,
@@ -825,10 +821,6 @@ Deno.test("process.exitCode in should change exit code", async () => {
  "import process from 'node:process'; process.exitCode = '0x10';",
  16,
  );
- await exitCodeTest(
- "import process from 'node:process'; process.exitCode = NaN;",
- 1,
- );
 });
 
 Deno.test("Deno.exit should override process exit", async () => {