Repository: jashkenas/underscore
Pull request #2829 (2020-02-29 15:15:56)

Fix for infinite recursion bug in overriding _.iteratee
The [documentation](https://underscorejs.org/#iteratee) states the following about `_.iteratee`:

> You may overwrite `_.iteratee` with your own custom function, if you want additional or different shorthand syntaxes:
>
> ```js
> // Support `RegExp` predicate shorthand.
> var builtinIteratee = _.iteratee;
> _.iteratee = function(value, context) {
>   if (_.isRegExp(value)) return function(obj) { return value.test(obj) };
>   return builtinIteratee(value, context);
> };
> ```

While working on #2826, I noticed a logical error in the implementation of this functionality, which basically meant that the above code example could never work as advertised.

Internally, most functions that work with `_.iteratee` call `cb`. The latter function first checks whether `_.iteratee` has been overridden, and if this is the case, immediately returns the result of `_.iteratee` instead of proceeding:

https://github.com/jashkenas/underscore/blob/b308ae36fe2ff5d19fa942fd3c40a5a2b807806a/underscore.js#L92-L93

By itself, this is reasonable, but the problem is that the `builtinIteratee`, which is initially exported as `_.iteratee` and which overriding implementations might fall back to, in turn calls `cb` again:

https://github.com/jashkenas/underscore/blob/b308ae36fe2ff5d19fa942fd3c40a5a2b807806a/underscore.js#L103-L105

So if the user overrides `_.iteratee` and attempts to fall back on the original iteratee in some cases, as in the example above, this will result in an infinite recursion where the overridden `_.iteratee` attempts to defer to the internal `cb` and vice versa.

I added some lines to the `_.iteratee` test to expose this bug. This resulted in "maximum call stack size exceeded" errors, which confirmed my suspicion. I then refactored `cb` and `_.iteratee` to fix the bug. Now all the tests pass again.

@jashkenas I think it's probably best to review (and hopefully merge) this one before #2826, so that if the latter breaks any backwards compatibility (although I hope and believe we can keep that to a minimum), people stuck with incompatible systems will still benefit from this fix. Again, I'd be very grateful if you could find the time to review this.


Patch:
diff --git a/underscore.js b/underscore.js
index 5cdf62e..0fff5f2 100644
--- a/underscore.js
+++ b/underscore.js
@@ -84,13 +84,10 @@
     };
   };
 
-  var builtinIteratee;
-
   // An internal function to generate callbacks that can be applied to each
   // element in a collection, returning the desired result — either `identity`,
   // an arbitrary callback, a property matcher, or a property accessor.
-  var cb = function(value, context, argCount) {
-    if (_.iteratee !== builtinIteratee) return _.iteratee(value, context);
+  var baseIteratee = function(value, context, argCount) {
     if (value == null) return _.identity;
     if (_.isFunction(value)) return optimizeCb(value, context, argCount);
     if (_.isObject(value) && !_.isArray(value)) return _.matcher(value);
@@ -100,8 +97,15 @@
   // External wrapper for our callback generator. Users may customize
   // `_.iteratee` if they want additional predicate/iteratee shorthand styles.
   // This abstraction hides the internal-only argCount argument.
-  _.iteratee = builtinIteratee = function(value, context) {
-    return cb(value, context, Infinity);
+  var exportIteratee = _.iteratee = function(value, context) {
+    return baseIteratee(value, context, Infinity);
+  };
+
+  // The function we actually call internally. It invokes _.iteratee if
+  // overridden, otherwise baseIteratee.
+  var cb = function(value, context, argCount) {
+    if (_.iteratee !== exportIteratee) return _.iteratee(value, context);
+    return baseIteratee(value, context, argCount);
   };
 
   // Some functions take a variable number of arguments, or a few expected


Test patch:
diff --git a/test/functions.js b/test/functions.js
index 696edbd..0418e67 100644
--- a/test/functions.js
+++ b/test/functions.js
@@ -701,12 +701,12 @@
 
     // Test custom iteratee
     var builtinIteratee = _.iteratee;
-    _.iteratee = function(value) {
+    _.iteratee = function(value, context) {
       // RegEx values return a function that returns the number of matches
       if (_.isRegExp(value)) return function(obj) {
         return (obj.match(value) || []).length;
       };
-      return value;
+      return builtinIteratee(value, context);
     };
 
     var collection = ['foo', 'bar', 'bbiz'];
@@ -734,6 +734,17 @@
     var objCollection = {a: 'foo', b: 'bar', c: 'bbiz'};
     assert.deepEqual(_.mapObject(objCollection, /b/g), {a: 0, b: 1, c: 2});
 
+    // Ensure that the overridden iteratee can still fall back on the builtin
+    // iteratee.
+    assert.strictEqual(_.iteratee(), _.identity);
+    assert.deepEqual(_.toArray(_.iteratee(fn)(1, 2, 3)), _.range(1, 4));
+    var matcher = _.iteratee({b: 'bar'});
+    assert.equal(matcher(objCollection), true);
+    assert.equal(matcher({}), false);
+    var property = _.iteratee('b');
+    assert.equal(property(objCollection), 'bar');
+    assert.equal(property({}), undefined);
+
     // Restore the builtin iteratee
     _.iteratee = builtinIteratee;
   });