Out of memory error is raised when invalid RegExp pattern string is provided as String#match argument

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yusuke Suzuki, Assigned: luke)

Tracking

Trunk
mozilla16
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox14-, firefox15-)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 631807 [details] [diff] [review]
v1.patch

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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #631807 - Attachment is patch: true
Attachment #631807 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 763372
(Assignee)

Updated

5 years ago
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
(Assignee)

Comment 2

5 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

5 years ago
Created attachment 632031 [details] [diff] [review]
fix and test

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

5 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 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

5 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).
> 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

5 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

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/15d7e7cc09a6
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/15d7e7cc09a6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Please re-nominate for tracking if this is believed to be a recent regression or has significant user impact.
tracking-firefox14: ? → -
tracking-firefox15: ? → -
You need to log in before you can comment on or make changes to this bug.