Bug 155679 – Crash in stress/regexp-matches-array-slow-put.js due to stomping on memory when having bad time

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

When we allocate the matches array in createRegExpMatchesArray() when globalObject->isHavingABadTime(), we don't allocate out of line storage needed for the match start index and source string.  This results on stepping on whatever object is immediately prior to the butterfly allocated for the array.  When testing a change I was working on, this caused a crash in stress/regexp-matches-array-slow-put.js.  The memory being stepped on all depends on allocation patterns.

[Radar WebKit Bug Importer]

<rdar://problem/25254505>

[Michael Saboff]

Created attachment 274534 [details]
Patch

[Michael Saboff]

Created attachment 274535 [details]
Performance results of the patch

Performance overall is neutral.  The results on the major benchmarks appear unchanged.  There are a few individual regression tests that show regression or slight progression.  I ran the tests s few times to validate the regressions.

[Saam Barati]

Comment on attachment 274534 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274534&action=review

> Source/JavaScriptCore/runtime/JSArray.h:243
> +    unsigned outOfLineStorage = structure->outOfLineCapacity();

Can we add a test to go with this? What about the having a bad time richards that we spoke about?

> Source/JavaScriptCore/runtime/JSArray.h:244
> +

Or maybe it can just be another test that more directly stresses this.

[Michael Saboff]

(In reply to comment #4)
> Comment on attachment 274534 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274534&action=review
> 
> > Source/JavaScriptCore/runtime/JSArray.h:243
> > +    unsigned outOfLineStorage = structure->outOfLineCapacity();
> 
> Can we add a test to go with this? What about the having a bad time richards
> that we spoke about?
> 
> > Source/JavaScriptCore/runtime/JSArray.h:244
> > +
> 
> Or maybe it can just be another test that more directly stresses this.

I tried modifying several tests and the best that I could do is change the failure of the test, but I couldn't any of those modified tests to crash.  First off, the bug is triggered by executing a regular expression.  That rules out richards and most other benchmark tests.  I tried both the Octane and SunSpider regexp tests, but could only get bad results and no crashing.

Remember, the bug is that createRegExpMatchesArray() is writing two JSValues in the 16 bytes right before the allocated array storage.  Those values are a String and a number.  They are both valid values overwriting other values.  To get a crash, the prior object in memory must crash due to those writes.

We do have a test that will crash after I land the RegExp.prototype[@@match] change.  I think that is good enough.

[Michael Saboff]

Committed r198478: <http://trac.webkit.org/changeset/198478>

[Saam Barati]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 274534 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=274534&action=review
> > 
> > > Source/JavaScriptCore/runtime/JSArray.h:243
> > > +    unsigned outOfLineStorage = structure->outOfLineCapacity();
> > 
> > Can we add a test to go with this? What about the having a bad time richards
> > that we spoke about?
> > 
> > > Source/JavaScriptCore/runtime/JSArray.h:244
> > > +
> > 
> > Or maybe it can just be another test that more directly stresses this.
> 
> I tried modifying several tests and the best that I could do is change the
> failure of the test, but I couldn't any of those modified tests to crash. 
> First off, the bug is triggered by executing a regular expression.  That
> rules out richards and most other benchmark tests.  I tried both the Octane
> and SunSpider regexp tests, but could only get bad results and no crashing.
> 
> Remember, the bug is that createRegExpMatchesArray() is writing two JSValues
> in the 16 bytes right before the allocated array storage.  Those values are
> a String and a number.  They are both valid values overwriting other values.
> To get a crash, the prior object in memory must crash due to those writes.
> 
> We do have a test that will crash after I land the RegExp.prototype[@@match]
> change.  I think that is good enough.

Right, I realized that richards was a bad idea after I posted. I think it's worth having a version of octane/regexp with having a bad time checked in.