Closed Bug 1168416 Opened 9 years ago Closed 9 years ago

Regexp.prototype.test/exec can silently change the value of a non-configurable readonly own property

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: bzbarsky, Assigned: arai)

Details

Attachments

(1 file)

Consider this testcase:

  var regex = /0/g;
  Object.freeze(regex);
  var str = "abc000";
  print(JSON.stringify(Object.getOwnPropertyDescriptor(regex, "lastIndex")));
  regex.test(str);
  print(JSON.stringify(Object.getOwnPropertyDescriptor(regex, "lastIndex")));

This prints out:

  {"value":0,"writable":false,"enumerable":false,"configurable":false}
  {"value":4,"writable":false,"enumerable":false,"configurable":false}

which is highly questionable.

The same issue exists with exec().
Assignee: nobody → arai.unmht
Added similar code to StringRegExpGuard::zeroLastIndex. This time we know it will always throw if it's not writable, so ReportLastIndexReadOnly calls JS_ReportErrorNumber directly, with string representation of "lastIndex" property.

Also, renumbered the spec steps to match ES6 draft.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bc86c60bae6
Attachment #8610827 - Flags: review?(jwalden+bmo)
Comment on attachment 8610827 [details] [diff] [review]
Do not change lastIndex property if not writable in RegExp.prototype.{test,exec}.

Review of attachment 8610827 [details] [diff] [review]:
-----------------------------------------------------------------

My eyes started glazing over after the "Steps 4-5" bit, from seeing our algorithm really isn't at all the ES6 one.  We should probably fix this all to be truly spec-compliant in a separate bug.  In the meantime, please add some sort of comment near the start of the method indicating that, notwithstanding the presence of step comments, this isn't the standard algorithm, maybe like so:

    /*
     * WARNING: Despite the presence of spec step comment numbers, this
     *          algorithm isn't consistent with any ES6 version, draft or
     *          otherwise.  YOU HAVE BEEN WARNED.
     */

::: js/src/builtin/RegExp.cpp
@@ +701,5 @@
>      return proto;
>  }
>  
> +static inline bool
> +ReportLastIndexReadOnly(JSContext* cx)

Just static bool, compilers will figure out the rest if they think it's the right choice, and they have better knowledge here than we do.

s/ReadOnly/Nonwritable/ because the spec uses "writable" as the term.

@@ +708,5 @@
> +    return false;
> +}
> +
> +static bool
> +ZeroLastIndex(JSContext* cx, Handle<RegExpObject*> reobj)

Could you just use SetLastIndex(..., 0) instead?  doesn't seem that important to have a method specially for this.

@@ +726,5 @@
> +        reobj->setLastIndex(lastIndex);
> +        return true;
> +    }
> +
> +    return ReportLastIndexReadOnly(cx);

Mild preference for

  if (!writable)
    return ReportLastIndexNonwritable(cx);

@@ +734,2 @@
>  RegExpRunStatus
>  js::ExecuteRegExp(JSContext* cx, HandleObject regexp, HandleString string,

Hmm, we should rename this to RegExpBuiltinExec eventually.  Separate patch.

@@ +734,5 @@
>  RegExpRunStatus
>  js::ExecuteRegExp(JSContext* cx, HandleObject regexp, HandleString string,
>                    MatchPairs* matches, RegExpStaticsUpdate staticsUpdate)
>  {
> +    /* Step 1 was performed by CallNonGenericMethod. */

Rather something like /* Steps 1-2 performed by the caller. */.  CNGM isn't necessarily the only way this guarantee could happen, as the spec sets up the algorithms.  And it doesn't really matter to the reader of this code *how* it happened, only that it's accurate.

@@ +739,1 @@
>      Rooted<RegExpObject*> reobj(cx, &regexp->as<RegExpObject>());

We should force the caller to provide a Handle<RegExpObject>, in a separate patch, just as the string is necessarily a string as enforced by the signature.

@@ +762,1 @@
>      /* Step 4. */

Steps 4-5.
Attachment #8610827 - Flags: review?(jwalden+bmo) → review+
Thank you for reviewing :D

Filed as bug 1171661.

I'll land this patch shortly.
https://hg.mozilla.org/mozilla-central/rev/e423430262c8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: