Closed
Bug 763384
Opened 9 years ago
Closed 9 years ago
Out of memory error is raised when invalid RegExp pattern string is provided as String#match argument
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: utatane.tea, Assigned: luke)
References
Details
Attachments
(1 file, 1 obsolete file)
2.96 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.54 Safari/536.5 Steps to reproduce: execute following script function test() { try { ''.match('('); } catch (e) { return true; } } Actual results: uncatchable out of memory error is raised Expected results: ''.match('(') raise SyntaxErorr, error is catched and returns true.
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•9 years ago
|
Attachment #631807 -
Attachment is patch: true
Updated•9 years ago
|
Attachment #631807 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Updated•9 years ago
|
tracking-firefox14:
--- → ?
tracking-firefox15:
--- → ?
![]() |
Assignee | |
Comment 2•9 years ago
|
||
Thanks for providing a clear test case and a patch as well! I looked into this a bit more and I think the underlying problem isn't that there is a missing syntax check but, rather, that the syntax error is getting clobbered by the out of memory error: http://hg.mozilla.org/mozilla-central/file/b7dd74f5a7d2/js/src/vm/RegExpObject.cpp#l567
![]() |
Assignee | |
Comment 3•9 years ago
|
||
Another 'goto' bites the dust...
Assignee: general → luke
Attachment #631807 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #631807 -
Flags: review?(jwalden+bmo)
Attachment #632031 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Comment 4•9 years ago
|
||
Comment on attachment 632031 [details] [diff] [review] fix and test Actually, this isn't regexp-specific, it's a general error and njn owes me a review or two ;)
Attachment #632031 -
Flags: review?(jwalden+bmo) → review?(n.nethercote)
![]() |
||
Comment 5•9 years ago
|
||
Comment on attachment 632031 [details] [diff] [review] fix and test Review of attachment 632031 [details] [diff] [review]: ----------------------------------------------------------------- I do owe you a review or two. And I just learnt about mfbt/Scoped.h, nice. ::: js/public/Utility.h @@ +597,5 @@ > > +template <typename T> > +struct ScopedDeleteTraits > +{ > + typedef T *type; Is this typedef necessary? I see that ScopedFreePtr has it but ScopedDelete{Ptr,Array} do not... @@ +601,5 @@ > + typedef T *type; > + static T *empty() { return NULL; } > + static void release(T *ptr) { Foreground::delete_(ptr); } > +}; > +SCOPED_TEMPLATE(ScopedDelete, ScopedDeleteTraits) Adding a "Ptr" suffix would be more consistent with the names in mfbt/Scoped.h. But then we'd have two subtly different classes with the same name. "ScopedForegroundDeletePtr" is a mouthful but very clear, and would be best in this case, IMO. ::: js/src/vm/RegExpObject.cpp @@ +564,2 @@ > if (!shared) > + return false; This case used to call js_ReportOutOfMemory(), but now it doesn't. Is that intentional? @@ +575,5 @@ > > /* > * Since 'error' deletes 'shared', only guard 'shared' on success. This is > * safe since 'shared' cannot be deleted by GC until after the call to > * map_.add() directly above. Change |map_.add()| to |map_.relookupOrAdd()|, plz.
Attachment #632031 -
Flags: review?(n.nethercote) → review+
![]() |
Assignee | |
Comment 6•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #5) > Is this typedef necessary? I see that ScopedFreePtr has it but > ScopedDelete{Ptr,Array} do not... Obscuriously, ScopedDeletePtrTraits gets it by deriving ScopedFreePtrTraits. > Adding a "Ptr" suffix would be more consistent with the names in > mfbt/Scoped.h. But then we'd have two subtly different classes with the > same name. "ScopedForegroundDeletePtr" is a mouthful but very clear, and > would be best in this case, IMO. SM won't be able to use any of templates in Scoped.h (b/c we can't call delete/free directly), so it shouldn't be an actual conflict in practice. The "Foreground" distinction is kindof lame (I there was a bug to remove the background-free thread... maybe it has already happened?), so I'd really not like to embed "Foreground" in even more names. ScopedDeletePtr sounds good, though. > ::: js/src/vm/RegExpObject.cpp > @@ +564,2 @@ > > if (!shared) > > + return false; > > This case used to call js_ReportOutOfMemory(), but now it doesn't. Is that > intentional? Note the change from cx->runtime->new_ to cx->new_ (so js_ReportOutOfMemory is still called).
![]() |
||
Comment 7•9 years ago
|
||
> Note the change from cx->runtime->new_ to cx->new_ (so js_ReportOutOfMemory
> is still called).
I did see that but I didn't realize the two functions had different semantics! How odd, and a little distasteful. Oh well.
![]() |
Assignee | |
Comment 8•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #7) Agreed. The current allocation naming scheme isn't quite right (for several reasons) and needs rejiggering.
![]() |
Assignee | |
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/15d7e7cc09a6
Target Milestone: --- → mozilla16
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/15d7e7cc09a6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 11•9 years ago
|
||
Please re-nominate for tracking if this is believed to be a recent regression or has significant user impact.
You need to log in
before you can comment on or make changes to this bug.
Description
•