380886 - Possible crashes/leaks in regexp handling in OOM conditions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gavin Reaney]

Attachment: Fixes for issues described in comment 0

There are a few cases where OOM can lead to leaks/crashes in regexp handling. These are outlined below:

(1) In ParseRegExp operatorStack and operandStack are leaked if the realloc fails (I'm sure Brian noted this before).

(2) ParseMinMaxQuantifier returns JS_FALSE if NewRENode fails, which ParseQuantifier interprets as success (leading to a crash).

(3) There's no check for failure of NewRENode in js_NewRegExp which can lead to a crash.

(4) If match_or_replace calls replace_glob it's possible for the memory allocated by replace_glob to be leaked in some error cases.

(5) When match_or_replace creates a new reg exp, it can be leaked if a later part of the function fails and KEEP_REGEXP isn't set.

I've uploaded some diffs to address these issues. Brian> I'll probably ask you to review fairly soon (when I've finished testing).

Comment 1

[Gavin Reaney]

For bug verification purposes, the test procedure for each issue is:

Issue 1:

In jsregexp.c, define INITIAL_STACK_SIZE to 1 and recompile the js shell. Use your favourite debugger to set a breakpoint on the realloc calls within ParseRegExp and run the test shell.

Enter the following regexp:

/^#[0-9a-f]{1,2}$/i

Force the realloc to fail (possibly by making the requested amount of memory very large). Check that the cleanup code behaves correctly and that nothing is leaked.

Issue 2:

Use the same regexp as for issue 1 and set a breakpoint in ParseMinMaxQuantifier. Force a failure of NewRENode and check that the caller no longer crashes and nothing is leaked.

Issue 3:

Set a breakpoint on js_NewRegExp and enter the following expression into the js shell:

'_'.replace('_', '/');

When the breakpoint is hit, force a failure of NewRENode and verify that the function no longer crashes and cleans up correctly.

Issue 4:

Set a breakpoint on match_or_replace and enter the following expression into the js shell:

'abca'.replace(/a/g, 'b');

When the breakpoint is hit, step through the function and let 'glob' be called once. Force a failure before the second call to this function (e.g. in js_ExecuteRegExp) and verify that the allocated memory is now cleaned up.

Issue 5:

Set a breakpoint on match_or_replace and enter the following expression into the js shell:

'_'.replace('_', '/');

When the breakpoint is hit, step through the function and force a failure in js_ExecuteRegExp. Verify that the regexp is now cleaned up.

Comment 2

[Gavin Reaney]

Comment on attachment 265005 [details] [diff] [review]
Fixes for issues described in comment 0

Brian, this is ready now, but feel free to nominate another reviewer if you are busy.

Comment 3

[Brian Crowder]

Comment on attachment 265005 [details] [diff] [review]
Fixes for issues described in comment 0

Thanks, Gavin!  Out of curiosity, did these OOM conditions bite you in reality or did you discover them with a testing tool, or what?

Also, thanks for the whitespace cleanup.

Comment 4

[Gavin Reaney]

(In reply to comment #3)
> (From update of attachment 265005 [details] [diff] [review])
> Thanks, Gavin!  Out of curiosity, did these OOM conditions bite you in reality
> or did you discover them with a testing tool, or what?
> 
> Also, thanks for the whitespace cleanup.
> 

I never encountered these particular failures in real world usage. A testing tool which fakes malloc failures exposed issues 2, 4 and 5. Issues 1 and 3 were noticed when fixing other bugs.

We use the engine in an application which (in some configurations) has a fixed 4M heap so it isn't beyond the realms of possibility that we could have encountered a problem eventually.

I haven't quite got around to applying for CVS write access, so if you can commit the patch to the trunk at some point that would be great.

Comment 5

[Brian Crowder]

Sounds like a great tool, any chance you can share it?  I'll be glad to land this bug for you, but if you don't mind I'd also like to vouch for you for CVS write access.  Brendan will SR and then we only need a couple more SRs to get you going (iirc).

Comment 6

[Gavin Reaney]

(In reply to comment #5)
> Sounds like a great tool, any chance you can share it?  

I'll look into this. When I last checked, Picsel management weren't willing to release it. However, the basic operation of such a test tool isn't too complicated so I may be able to provide something basic for use with the JS shell.

> I'll be glad to land
> this bug for you, but if you don't mind I'd also like to vouch for you for CVS
> write access.  Brendan will SR and then we only need a couple more SRs to get
> you going (iirc).
> 

Great, thanks!

Comment 7

[timeless]

Comment on attachment 265005 [details] [diff] [review]
Fixes for issues described in comment 0

mozilla/js/src/jsregexp.c 	3.151
mozilla/js/src/jsstr.c 	3.143

Comment 8

[Gavin Reaney]

This was completed some time ago, so marking as fixed.