Fix handling very large stacks of sync middleware

Author: dougwilson

HISTORY.md

@@ -1,6 +1,7 @@
 unreleased
 ==========
 
+  * Fix handling very large stacks of sync middleware
   * deps: safe-buffer@5.2.1
 
 1.3.5 / 2020-03-24

index.js

@@ -163,6 +163,7 @@ Router.prototype.handle = function handle(req, res, callback) {
   var removed = ''
   var self = this
   var slashAdded = false
+  var sync = 0
   var paramcalled = {}
 
   // middleware and routes
@@ -218,6 +219,11 @@ Router.prototype.handle = function handle(req, res, callback) {
       return
     }
 
+    // max sync stack
+    if (++sync > 100) {
+      return defer(next, err)
+    }
+
     // get pathname of request
     var path = getPathname(req)
 
@@ -340,6 +346,8 @@ Router.prototype.handle = function handle(req, res, callback) {
     } else {
       layer.handle_request(req, res, next)
     }
+
+    sync = 0
   }
 }
 

lib/route.js

@@ -24,6 +24,11 @@ var methods = require('methods')
 
 var slice = Array.prototype.slice
 
+/* istanbul ignore next */
+var defer = typeof setImmediate === 'function'
+  ? setImmediate
+  : function (fn) { process.nextTick(fn.bind.apply(fn, arguments)) }
+
 /**
  * Expose `Route`.
  */
@@ -95,6 +100,8 @@ Route.prototype._methods = function _methods() {
 Route.prototype.dispatch = function dispatch(req, res, done) {
   var idx = 0
   var stack = this.stack
+  var sync = 0
+
   if (stack.length === 0) {
     return done()
   }
@@ -124,6 +131,11 @@ Route.prototype.dispatch = function dispatch(req, res, done) {
       return done(err)
     }
 
+    // max sync stack
+    if (++sync > 100) {
+      return defer(next, err)
+    }
+
     var layer
     var match
 
@@ -143,6 +155,8 @@ Route.prototype.dispatch = function dispatch(req, res, done) {
     } else {
       layer.handle_request(req, res, next)
     }
+
+    sync = 0
   }
 }
 

test/route.js

@@ -103,6 +103,24 @@ describe('Router', function () {
         .expect(404, done)
     })
 
+    it('should not stack overflow with a large sync stack', function (done) {
+      this.timeout(5000) // long-running test
+
+      var router = new Router()
+      var route = router.route('/foo')
+      var server = createServer(router)
+
+      for (var i = 0; i < 6000; i++) {
+        route.all(function (req, res, next) { next() })
+      }
+
+      route.get(helloWorld)
+
+      request(server)
+        .get('/foo')
+        .expect(200, 'hello, world', done)
+    })
+
     describe('.all(...fn)', function () {
       it('should reject no arguments', function () {
         var router = new Router()

test/router.js

@@ -577,6 +577,23 @@ describe('Router', function () {
         .expect(404, done)
     })
 
+    it('should not stack overflow with a large sync stack', function (done) {
+      this.timeout(5000) // long-running test
+
+      var router = new Router()
+      var server = createServer(router)
+
+      for (var i = 0; i < 6000; i++) {
+        router.use(function (req, res, next) { next() })
+      }
+
+      router.use(helloWorld)
+
+      request(server)
+        .get('/')
+        .expect(200, 'hello, world', done)
+    })
+
     describe('error handling', function () {
       it('should invoke error function after next(err)', function (done) {
         var router = new Router()