readline: validate AbortSignals and remove unused event listeners

PR-URL: #37947 Fixes: #37287 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
nodejs · Sep 16, 2021 · 707dd77 · 707dd77
1 parent 8122d24
commit 707dd77
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 9 deletions.
diff --git a/lib/readline.js b/lib/readline.js
@@ -48,6 +48,7 @@ const {
   inspect,
 } = require('internal/util/inspect');
 const { promisify } = require('internal/util');
+const { validateAbortSignal } = require('internal/validators');
 
 /**
  * @typedef {import('./stream.js').Readable} Readable
@@ -130,13 +131,22 @@ Interface.prototype.question = function(query, options, cb) {
   options = typeof options === 'object' && options !== null ? options : {};
 
   if (options.signal) {
+    validateAbortSignal(options.signal, 'options.signal');
     if (options.signal.aborted) {
       return;
     }
 
-    options.signal.addEventListener('abort', () => {
+    const onAbort = () => {
       this[kQuestionCancel]();
-    }, { once: true });
+    };
+    options.signal.addEventListener('abort', onAbort, { once: true });
+    const cleanup = () => {
+      options.signal.removeEventListener(onAbort);
+    };
+    cb = typeof cb === 'function' ? (answer) => {
+      cleanup();
+      return cb(answer);
+    } : cleanup;
   }
 
   if (typeof cb === 'function') {
@@ -151,13 +161,20 @@ Interface.prototype.question[promisify.custom] = function(query, options) {
   }
 
   return new Promise((resolve, reject) => {
-    this.question(query, options, resolve);
+    let cb = resolve;
 
     if (options.signal) {
-      options.signal.addEventListener('abort', () => {
+      const onAbort = () => {
         reject(new AbortError());
-      }, { once: true });
+      };
+      options.signal.addEventListener('abort', onAbort, { once: true });
+      cb = (answer) => {
+        options.signal.removeEventListener('abort', onAbort);
+        resolve(answer);
+      };
     }
+
+    this.question(query, options, cb);
   });
 };
 

diff --git a/lib/readline/promises.js b/lib/readline/promises.js
@@ -16,6 +16,7 @@ const {
 const {
   AbortError,
 } = require('internal/errors');
+const { validateAbortSignal } = require('internal/validators');
 
 class Interface extends _Interface {
   // eslint-disable-next-line no-useless-constructor
@@ -24,18 +25,26 @@ class Interface extends _Interface {
   }
   question(query, options = {}) {
     return new Promise((resolve, reject) => {
-      if (options.signal) {
+      let cb = resolve;
+
+      if (options?.signal) {
+        validateAbortSignal(options.signal, 'options.signal');
         if (options.signal.aborted) {
           return reject(new AbortError());
         }
 
-        options.signal.addEventListener('abort', () => {
+        const onAbort = () => {
           this[kQuestionCancel]();
           reject(new AbortError());
-        }, { once: true });
+        };
+        options.signal.addEventListener('abort', onAbort, { once: true });
+        cb = (answer) => {
+          options.signal.removeEventListener('abort', onAbort);
+          resolve(answer);
+        };
       }
 
-      super.question(query, resolve);
+      super.question(query, cb);
     });
   }
 }