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

Status: RESOLVED FIXED

Description

[Yusuke Suzuki]

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.

Comment 2

[Luke Wagner [:luke]]

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

Comment 3

[Luke Wagner [:luke]]

Created attachment 632031 [details] [diff] [review]
fix and test

Another 'goto' bites the dust...

Comment 4

[Luke Wagner [:luke]]

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 ;)

Comment 5

[Nicholas Nethercote [:njn]]

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.

Comment 6

[Luke Wagner [:luke]]

(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

[Nicholas Nethercote [:njn]]

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

Comment 8

[Luke Wagner [:luke]]

(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.

Comment 9

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15d7e7cc09a6

Comment 10

[Ed Morley (Away 27th-2nd) [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/15d7e7cc09a6

Comment 11

[Alex Keybl [:akeybl]]

Please re-nominate for tracking if this is believed to be a recent regression or has significant user impact.