From 708ac4cdf5cd0a658d62490a9f4d78d3e1ec6612 Mon Sep 17 00:00:00 2001 From: Douglas Christopher Wilson Date: Wed, 13 Apr 2022 23:29:25 -0400 Subject: [PATCH] Fix handling very large stacks of sync middleware closes #4891 --- History.md | 1 + lib/router/index.js | 8 ++++++++ lib/router/route.js | 9 +++++++++ test/Route.js | 22 ++++++++++++++++++++++ test/Router.js | 16 ++++++++++++++++ 5 files changed, 56 insertions(+) diff --git a/History.md b/History.md index 4f43ad9255..3f7851ba57 100644 --- a/History.md +++ b/History.md @@ -5,6 +5,7 @@ unreleased * Allow `options` without `filename` in `res.download` * Deprecate string and non-integer arguments to `res.status` * Fix behavior of `null`/`undefined` as `maxAge` in `res.cookie` + * Fix handling very large stacks of sync middleware * Ignore `Object.prototype` values in settings through `app.set`/`app.get` * Invoke `default` with same arguments as types in `res.format` * Support proper 205 responses using `res.send` diff --git a/lib/router/index.js b/lib/router/index.js index 791a600f86..f4c8c0a79e 100644 --- a/lib/router/index.js +++ b/lib/router/index.js @@ -142,6 +142,7 @@ proto.handle = function handle(req, res, out) { var protohost = getProtohost(req.url) || '' var removed = ''; var slashAdded = false; + var sync = 0 var paramcalled = {}; // store options for OPTIONS request @@ -203,6 +204,11 @@ proto.handle = function handle(req, res, out) { return; } + // max sync stack + if (++sync > 100) { + return setImmediate(next, err) + } + // get pathname of request var path = getPathname(req); @@ -321,6 +327,8 @@ proto.handle = function handle(req, res, out) { } else { layer.handle_request(req, res, next); } + + sync = 0 } }; diff --git a/lib/router/route.js b/lib/router/route.js index 178df0d516..5adaa125e2 100644 --- a/lib/router/route.js +++ b/lib/router/route.js @@ -98,6 +98,8 @@ Route.prototype._options = function _options() { Route.prototype.dispatch = function dispatch(req, res, done) { var idx = 0; var stack = this.stack; + var sync = 0 + if (stack.length === 0) { return done(); } @@ -127,6 +129,11 @@ Route.prototype.dispatch = function dispatch(req, res, done) { return done(err); } + // max sync stack + if (++sync > 100) { + return setImmediate(next, err) + } + if (layer.method && layer.method !== method) { return next(err); } @@ -136,6 +143,8 @@ Route.prototype.dispatch = function dispatch(req, res, done) { } else { layer.handle_request(req, res, next); } + + sync = 0 } }; diff --git a/test/Route.js b/test/Route.js index 8e7ddbdbcc..3bdc8d7df2 100644 --- a/test/Route.js +++ b/test/Route.js @@ -13,6 +13,28 @@ describe('Route', function(){ route.dispatch(req, {}, done) }) + it('should not stack overflow with a large sync stack', function (done) { + this.timeout(5000) // long-running test + + var req = { method: 'GET', url: '/' } + var route = new Route('/foo') + + for (var i = 0; i < 6000; i++) { + route.all(function (req, res, next) { next() }) + } + + route.get(function (req, res, next) { + req.called = true + next() + }) + + route.dispatch(req, {}, function (err) { + if (err) return done(err) + assert.ok(req.called) + done() + }) + }) + describe('.all', function(){ it('should add handler', function(done){ var req = { method: 'GET', url: '/' }; diff --git a/test/Router.js b/test/Router.js index 907b972636..8a0654bca3 100644 --- a/test/Router.js +++ b/test/Router.js @@ -76,6 +76,22 @@ describe('Router', function(){ router.handle({ url: '/', method: 'GET' }, { end: done }); }); + it('should not stack overflow with a large sync stack', function (done) { + this.timeout(5000) // long-running test + + var router = new Router() + + for (var i = 0; i < 6000; i++) { + router.use(function (req, res, next) { next() }) + } + + router.use(function (req, res) { + res.end() + }) + + router.handle({ url: '/', method: 'GET' }, { end: done }) + }) + describe('.handle', function(){ it('should dispatch', function(done){ var router = new Router();