Fix handling very large stacks of sync middleware

closes #4891

Author: dougwilson

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`

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
   }
 };
 

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
   }
 };
 

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: '/' };

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();