Possible crashes/leaks in regexp handling in OOM conditions

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Gavin Reaney, Assigned: Gavin Reaney)

Tracking

Trunk
x86
Windows XP
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
Created attachment 265005 [details] [diff] [review]
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).
(Assignee)

Comment 1

11 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 2

11 years ago
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.
Attachment #265005 - Flags: review?(crowder)

Comment 3

11 years ago
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.
Attachment #265005 - Flags: review?(crowder) → review+
(Assignee)

Comment 4

11 years ago
(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

11 years ago
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).
(Assignee)

Comment 6

11 years ago
(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!

Updated

11 years ago
Whiteboard: [needs landing]

Updated

11 years ago
Whiteboard: [needs landing] → [checkin needed]

Comment 7

11 years ago
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

Updated

11 years ago
Whiteboard: [checkin needed]
(Assignee)

Comment 8

9 years ago
This was completed some time ago, so marking as fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.