Update fuzz targets (#697)

* Get fuzz shell once per fuzz target * Handle errors explicitly in fuzz targets * Extract argument escaping/quoting out of invocation Should make for easier overall reading and aligns all fuzz targets with the style of `exec.test.cjs`. * Test both sync and callback versions of explicitly Since, per [1], it's not actually guaranteed these work the same. * Remove multiple argument versions of checks I do not believe these currently provide significant value. They don't seem to test anything special about multiple arguments. Plus the method by which these convert the buffer into multiple args leaves a lot to be desired. An upside is that fuzzing should become more efficient as less cycles are wasted on testing something with little to no value. -- 1. nodejs/node#43333
ericcornelissen · Feb 1, 2023 · 15c805b · 15c805b
1 parent 68ebdf5
commit 15c805b
Show file tree

Hide file tree

Showing 4 changed files with 188 additions and 140 deletions.
diff --git a/test/fuzz/exec-file.test.cjs b/test/fuzz/exec-file.test.cjs
@@ -5,74 +5,79 @@
  */
 
 const assert = require("node:assert");
-const { execFileSync } = require("node:child_process");
+const { execFile, execFileSync } = require("node:child_process");
 
 const common = require("./_common.cjs");
 
 const shescape = require("../../index.cjs");
 
-function check(arg) {
-  const shell = common.getFuzzShell();
+function check({ arg, shell }) {
   const argInfo = { arg, shell, quoted: Boolean(shell) };
   const execFileOptions = { encoding: "utf8", shell };
 
   const preparedArg = common.prepareArg(argInfo, !Boolean(shell));
+  const safeArg = execFileOptions.shell
+    ? shescape.quote(preparedArg, execFileOptions)
+    : shescape.escape(preparedArg, execFileOptions);
 
-  const stdout = execFileSync(
-    "node",
-    execFileOptions.shell
-      ? shescape.quoteAll([common.ECHO_SCRIPT, preparedArg], execFileOptions)
-      : shescape.escapeAll([common.ECHO_SCRIPT, preparedArg], execFileOptions),
-    execFileOptions
-  );
-
-  const result = stdout;
-  const expected = common.getExpectedOutput(argInfo);
-  assert.strictEqual(result, expected);
+  return new Promise((resolve, reject) => {
+    execFile(
+      "node",
+      [common.ECHO_SCRIPT, safeArg],
+      execFileOptions,
+      (error, stdout) => {
+        if (error) {
+          reject(`an unexpected error occurred: ${error}`);
+        } else {
+          const result = stdout;
+          const expected = common.getExpectedOutput(argInfo);
+          try {
+            assert.strictEqual(result, expected);
+            resolve();
+          } catch (e) {
+            reject(e);
+          }
+        }
+      }
+    );
+  });
 }
 
-function checkMultipleArgs(args) {
-  const shell = common.getFuzzShell();
-  const argInfo = { shell, quoted: Boolean(shell) };
+function checkSync({ arg, shell }) {
+  const argInfo = { arg, shell, quoted: Boolean(shell) };
   const execFileOptions = { encoding: "utf8", shell };
 
-  const preparedArgs = args.map((arg) =>
-    common.prepareArg({ ...argInfo, arg }, !Boolean(shell))
-  );
+  const preparedArg = common.prepareArg(argInfo, !Boolean(shell));
+  const safeArg = execFileOptions.shell
+    ? shescape.quote(preparedArg, execFileOptions)
+    : shescape.escape(preparedArg, execFileOptions);
 
-  const stdout = execFileSync(
-    "node",
-    execFileOptions.shell
-      ? shescape.quoteAll(
-          [common.ECHO_SCRIPT, ...preparedArgs],
-          execFileOptions
-        )
-      : shescape.escapeAll(
-          [common.ECHO_SCRIPT, ...preparedArgs],
-          execFileOptions
-        ),
-    execFileOptions
-  );
+  let stdout;
+  try {
+    stdout = execFileSync(
+      "node",
+      [common.ECHO_SCRIPT, safeArg],
+      execFileOptions
+    );
+  } catch (error) {
+    assert.fail(`an unexpected error occurred: ${error}`);
+  }
 
   const result = stdout;
-  const expected = common.getExpectedOutput({
-    ...argInfo,
-    arg: (common.isShellPowerShell(shell)
-      ? args.filter(
-          (arg) => arg.replace(/[\0\u0008\u001B\u009B]/gu, "").length !== 0
-        )
-      : args
-    ).join(" "),
-  });
+  const expected = common.getExpectedOutput(argInfo);
   assert.strictEqual(result, expected);
 }
 
-function fuzz(buf) {
+async function fuzz(buf) {
   const arg = buf.toString();
-  const args = arg.split(/[\n\r]+/u);
+  const shell = common.getFuzzShell();
 
-  check(arg);
-  checkMultipleArgs(args);
+  try {
+    await check({ arg, shell });
+    checkSync({ arg, shell });
+  } catch (e) {
+    throw e;
+  }
 }
 
 module.exports = {

diff --git a/test/fuzz/exec.test.cjs b/test/fuzz/exec.test.cjs
@@ -5,14 +5,13 @@
  */
 
 const assert = require("node:assert");
-const { execSync } = require("node:child_process");
+const { exec, execSync } = require("node:child_process");
 
 const common = require("./_common.cjs");
 
 const shescape = require("../../index.cjs");
 
-function check(arg) {
-  const shell = common.getFuzzShell();
+function check({ arg, shell }) {
   const argInfo = { arg, shell, quoted: true };
   const execOptions = { encoding: "utf8", shell };
 
@@ -21,18 +20,50 @@ function check(arg) {
     ...execOptions,
   });
 
-  const stdout = execSync(
-    `node ${common.ECHO_SCRIPT} ${quotedArg}`,
-    execOptions
-  );
+  return new Promise((resolve, reject) => {
+    exec(
+      `node ${common.ECHO_SCRIPT} ${quotedArg}`,
+      execOptions,
+      (error, stdout) => {
+        if (error) {
+          reject(`an unexpected error occurred: ${error}`);
+        } else {
+          const result = stdout;
+          const expected = common.getExpectedOutput(argInfo);
+          try {
+            assert.strictEqual(result, expected);
+            resolve();
+          } catch (e) {
+            reject(e);
+          }
+        }
+      }
+    );
+  });
+}
+
+function checkSync({ arg, shell }) {
+  const argInfo = { arg, shell, quoted: true };
+  const execOptions = { encoding: "utf8", shell };
+
+  const preparedArg = common.prepareArg(argInfo);
+  const quotedArg = shescape.quote(preparedArg, {
+    ...execOptions,
+  });
+
+  let stdout;
+  try {
+    stdout = execSync(`node ${common.ECHO_SCRIPT} ${quotedArg}`, execOptions);
+  } catch (error) {
+    assert.fail(`an unexpected error occurred: ${error}`);
+  }
 
   const result = stdout;
   const expected = common.getExpectedOutput(argInfo);
   assert.strictEqual(result, expected);
 }
 
-function checkUsingInterpolation(arg) {
-  const shell = common.getFuzzShell();
+function checkUsingInterpolation({ arg, shell }) {
   const argInfo = { arg, shell, quoted: false };
   const execOptions = { encoding: "utf8", shell };
 
@@ -42,21 +73,62 @@ function checkUsingInterpolation(arg) {
     interpolation: true,
   });
 
-  const stdout = execSync(
-    `node ${common.ECHO_SCRIPT} ${escapedArg}`,
-    execOptions
-  );
+  return new Promise((resolve, reject) => {
+    exec(
+      `node ${common.ECHO_SCRIPT} ${escapedArg}`,
+      execOptions,
+      (error, stdout) => {
+        if (error) {
+          reject(`an unexpected error occurred: ${error}`);
+        } else {
+          const result = stdout;
+          const expected = common.getExpectedOutput(argInfo, true);
+          try {
+            assert.strictEqual(result, expected);
+            resolve();
+          } catch (e) {
+            reject(e);
+          }
+        }
+      }
+    );
+  });
+}
+
+function checkUsingInterpolationSync({ arg, shell }) {
+  const argInfo = { arg, shell, quoted: false };
+  const execOptions = { encoding: "utf8", shell };
+
+  const preparedArg = common.prepareArg(argInfo);
+  const escapedArg = shescape.escape(preparedArg, {
+    ...execOptions,
+    interpolation: true,
+  });
+
+  let stdout;
+  try {
+    stdout = execSync(`node ${common.ECHO_SCRIPT} ${escapedArg}`, execOptions);
+  } catch (error) {
+    assert.fail(`an unexpected error occurred: ${error}`);
+  }
 
   const result = stdout;
   const expected = common.getExpectedOutput(argInfo, true);
   assert.strictEqual(result, expected);
 }
 
-function fuzz(buf) {
+async function fuzz(buf) {
   const arg = buf.toString();
+  const shell = common.getFuzzShell();
 
-  check(arg);
-  checkUsingInterpolation(arg);
+  try {
+    await check({ arg, shell });
+    await checkUsingInterpolation({ arg, shell });
+    checkSync({ arg, shell });
+    checkUsingInterpolationSync({ arg, shell });
+  } catch (e) {
+    throw e;
+  }
 }
 
 module.exports = {

diff --git a/test/fuzz/fork.test.cjs b/test/fuzz/fork.test.cjs
@@ -16,48 +16,18 @@ function check(arg) {
   const forkOptions = { silent: true };
 
   const preparedArg = common.prepareArg(argInfo, true);
+  const safeArg = shescape.escape(preparedArg);
 
   return new Promise((resolve, reject) => {
-    const echo = fork(
-      common.ECHO_SCRIPT,
-      shescape.escapeAll([preparedArg]),
-      forkOptions
-    );
+    const echo = fork(common.ECHO_SCRIPT, [safeArg], forkOptions);
 
-    echo.stdout.on("data", (data) => {
-      const result = data.toString();
-      const expected = common.getExpectedOutput(argInfo);
-      try {
-        assert.strictEqual(result, expected);
-        resolve();
-      } catch (e) {
-        reject(e);
-      }
+    echo.on("error", (error) => {
+      reject(`an unexpected error occurred: ${error}`);
     });
-  });
-}
-
-function checkMultipleArgs(args) {
-  const argInfo = { quoted: false };
-  const forkOptions = { silent: true };
-
-  const preparedArgs = args.map((arg) =>
-    common.prepareArg({ ...argInfo, arg }, true)
-  );
-
-  return new Promise((resolve, reject) => {
-    const echo = fork(
-      common.ECHO_SCRIPT,
-      shescape.escapeAll(preparedArgs),
-      forkOptions
-    );
 
     echo.stdout.on("data", (data) => {
       const result = data.toString();
-      const expected = common.getExpectedOutput({
-        ...argInfo,
-        arg: args.join(" "),
-      });
+      const expected = common.getExpectedOutput(argInfo);
       try {
         assert.strictEqual(result, expected);
         resolve();
@@ -70,11 +40,9 @@ function checkMultipleArgs(args) {
 
 async function fuzz(buf) {
   const arg = buf.toString();
-  const args = arg.split(/[\n\r]+/u);
 
   try {
     await check(arg);
-    await checkMultipleArgs(args);
   } catch (e) {
     throw e;
   }