#469 ✓resolved
michael (at mdaines)

String#gsub should RegExp.escape its input

Reported by michael (at mdaines) | November 27th, 2008 @ 07:15 PM | in 1.6.1

The following both cause an infinite loop:

"ab|cd".gsub("|", "");
"ab|cd".gsub(/|/, "");

I usually expect String#gsub to work like String#replace (with the "g" flag). When used with similar arguments, String#replace happily erases the vertical bar (first case) or leaves it alone (second case).

I can't entirely discern what the behaviour of String#gsub is supposed to be when a string pattern is used. Is it supposed to be interpreted as a string (a string with a vertical bar being equivalent to an escaped vertical bar in a regular expression) or as though it were a regular expression created with new RegExp? There don't seem to be any tests that give a string argument to String#gsub for the pattern.

Should I just use String#replace in this case? Try and write a patch?

Comments and changes to this ticket

  • Juriy Zaytsev

    Juriy Zaytsev November 28th, 2008 @ 12:09 AM

    • Milestone set to 1.7
    • State changed from “new” to “bug”
    • Assigned user set to “Juriy Zaytsev”

    Looks like a bug in gsub to me. A pattern string needs to be escaped with RegExp.escape before attempting an actual match. Same behavior happens when passing an empty string:

    'foo'.gsub('', ''); // freezes

    gsub also throws syntax errors when pattern contains quantifiers-like characters - '*', '+', '?', '{1}'

    If replacement is a string (as in your case), you should use native String.prototype.replace passing it a plain regexp with a 'g' flag (replace is up to ~30 times faster than gsub)

    'ab|cd'.replace(/\|/g, '');

    If replacement is a function, and an app is intended to work in older browsers (in which String.prototype.replace doesn't work with function as a replacement) use gsub but escape pattern with RegExp.escape.

    If replacement is a template-like string, use gsub (which interpolates those strings automatically)

    I'll make a patch for this.

  • Juriy Zaytsev

    Juriy Zaytsev November 29th, 2008 @ 04:27 AM

    A bit more testing reveals few other similar problems:

    // Freezes (when pattern is an empty/broken regexp)
    'a|b'.gsub('|', ''); // freeze
    'ab'.gsub('', ''); // freeze
    'ab'.gsub('(?:)', ''); // freeze
    'ab'.gsub('()', ''); // freeze
    'ab'.gsub('^', ''); // freeze
    // Throws errors (when pattern contains only chars which are quantifiers in regexp grammar)
    'a?b'.gsub('?', ''); // Error
    'a+b'.gsub('+', ''); // Error
    'a*b'.gsub('*', ''); // Error
    'a{1}b'.gsub('{1}', ''); // Error
    // Wrong behavior (?)
    'a.b'.gsub('.', ''); // deletes all characters
  • Juriy Zaytsev

    Juriy Zaytsev December 6th, 2008 @ 02:11 AM

    • Title changed from “Vertical bar passed to String#gsub as pattern causes infinite loop” to “String#gsub should RegExp.escape its input”
  • Juriy Zaytsev
  • Tobie Langel

    Tobie Langel December 6th, 2008 @ 06:24 PM

    Kangax, I suspect the easiest thing would be to attach a diff file here.

  • Juriy Zaytsev
  • Juriy Zaytsev

    Juriy Zaytsev February 22nd, 2009 @ 10:42 PM

    • Tag set to patched, tested
    • Assigned user changed from “Juriy Zaytsev” to “Andrew Dupont”
    • Milestone changed from 1.7 to 1.6.1
  • GitHub Robot

    GitHub Robot February 24th, 2009 @ 03:21 AM

    • State changed from “bug” to “resolved”

    (from [e9bdaef0afc407ea3220ad39d69d88731d8d06fa]) String#gsub should escape RegExp metacharacters when the first argument is a string. [#469 state:resolved] (michael, kangax) http://github.com/sstephenson/pr...

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

The Prototype JavaScript library.

Shared Ticket Bins



Referenced by