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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: arai)
Details
Attachments
(1 file)
5.98 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•9 years ago
|
Assignee: nobody → arai.unmht
Assignee | ||
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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, ®exp->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+
Assignee | ||
Comment 3•9 years ago
|
||
Thank you for reviewing :D Filed as bug 1171661. I'll land this patch shortly.
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e423430262c8
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•