repl: preventing recursive eval of RegEx predefined properties

[thefourtheye]

Fixes: #597

In REPL, if we evaluate the RegExp object's predefined properties,
and if they happen to have the same expression, for example,

> RegExp.$1
'RegExp.$1'

then doing eval(RegExp.$1) would evaluate RegExp.$1 recursively and
eventually throw RangeError: Maximum call stack size exceeded.

This patch stores the old values of RegExp's predefined properties and
restores them just before the current expression entered by user is
evaluated.

[thefourtheye]

bump

[Fishrock123]

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/162/

[thefourtheye]

Here also, none of the failures are due to this change :-)

[Fishrock123]

Could this be just \u0000(.*)? I'm trying to understand how this actually works.

[Fishrock123]

Hmm reading ben's comment (#597 (comment))

Aside: using '\u0000' as the separator is probably not unique enough in practice.

[thefourtheye]

@Fishrock123 I had to rebase with master, so force pushed the commits. PTAL now.

[Fishrock123]

// Regression test

[Fishrock123]

SGTM but I'm not comfortable stamping it given I don't fully get how the regex works haha. :S

[thefourtheye]

@Fishrock123 The RegExp.${1,9} are read-only properties. The only way to change their values is by matching a string with a regular expression. So, we have an array savedRegExMatches, where the values of RegExp properties will be stored. To store the values of RegExp properties, we just iterate with the for loop and store the value in the corresponding index. To re-establish the values, we join the values in savedRegExMatches with the delimiter \u0000 and then match that string with a regular expression which captures any value between one delimiter to another. So, the captured values are restored in the RegExp properties.

[thefourtheye]

cc @chrisdickinson @bnoordhuis

[bnoordhuis]

These two tests depend on the behavior of previous tests, don't they? Seems a little fragile.

[thefourtheye]

Even I thought so, that is why I included .clear in the first test. Shall I move these tests to the beginning?

[bnoordhuis]

You could replace .clear with something like /(.)(.)(.)(.)(.)(.)(.)(.)(.)/.exec('123456789') and then check that RegExp.$1 to RegExp.$9 have the expected values.

And can I suggest you prefix the second test with a comment explaining it depends on the result of the previous one?

[thefourtheye]

@bnoordhuis Oh yeah, that would make more sense. Thanks :-) Updated the test. PTAL.

[bnoordhuis]

I think the comment about \u0000 not being unique enough should still be addressed but LGTM apart from that.

[thefourtheye]

@bnoordhuis No string can be unique enough to be used as a separator, right?

[bnoordhuis]

A string of infinite length can. Facetiousness aside, I'd pick a slightly longer separator. Every character lowers the odds of accidental collisions significantly.

[thefourtheye]

New CI run: iojs+any-pr+multi/220 Ubuntu 15.04_64 box is failing without testing. I ll start the CI run when it is up.

[thefourtheye]

New CI Run: node-test-commit/84/

[thefourtheye]

Failed test is not related to this change. @bnoordhuis @Fishrock123 LGTY?

[bnoordhuis]

LGTM but the first line of the commit log should be <= 50 characters. I'd make it something like repl: don't clobber RegExp.$ properties).

In the commit log itself, s/evluated/evaluate/ and the Fixes: tag should go at the bottom, right before the PR-URL tag.

[thefourtheye]

Thanks guys. Landed at ea05e76