free unitialized memory from js_DestroyRegExp in OOM conditions

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Gavin Reaney, Assigned: Gavin Reaney)

Tracking

({fixed1.8.0.10, fixed1.8.1.2})

Trunk
x86
Windows XP
fixed1.8.0.10, fixed1.8.1.2
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
This was noticed when running some OOM testing. The failure point is indicated below:

js_DestroyRegExp(JSContext *cx, JSRegExp *re)
{
    if (JS_ATOMIC_DECREMENT(&re->nrefs) == 0) {
        if (re->classList) {
            uintN i;
            for (i = 0; i < re->classCount; i++) {
                if (re->classList[i].converted)
                    JS_free(cx, re->classList[i].u.bits);  //<-- double free
                re->classList[i].u.bits = NULL;
            }
            JS_free(cx, re->classList);
        }
        JS_free(cx, re);
    }
}

This is caused when js_NewRegExp attempts to destroy a partially constructed regexp (the memory allocated to re->classlist is left uninitialised if EmitREBytecode fails).
(Assignee)

Comment 1

11 years ago
Created attachment 249106 [details] [diff] [review]
Fix for double free

Proposed fix - initialise the class list to allow safe destruction.
Attachment #249106 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
Brian, can you shepherd this patch in?

Gavin: thanks again. If you want to apply for CVS access, I'll vouch for you.

/be

Comment 3

11 years ago
Brendan:  Is that your r+?

Updated

11 years ago
Attachment #249106 - Flags: review?(brendan) → review?(crowder)

Comment 4

11 years ago
Any thoughts or testing on whether this performs better/worse as a memset()?
(In reply to comment #4)
> Any thoughts or testing on whether this performs better/worse as a memset()?

Good point -- could be both faster and less code (code size comparison welcome too).

/be
(Assignee)

Comment 6

11 years ago
(In reply to comment #5)
> (In reply to comment #4)
> > Any thoughts or testing on whether this performs better/worse as a memset()?
> 
> Good point -- could be both faster and less code (code size comparison welcome
> too).
> 
> /be

I was in two minds about using memset - I can certainly change it if you think this would be better.

My main reason for not using memset was to make the required initialisation more explicit - js_DestroyRegExp checks for 'converted' being JS_FALSE so that is what is set during initialisation.

It's not exactly an overwhelming argument though since memset(0) is such a common idiom that its purpose should be clear. It's also more future proof should we later require other members of the structure to be zero'd.

In terms of code size, the final binary (for the js shell) is the same size when memset is used instead (build with -0 and no debug). This was with 'gcc version 3.3.5 (Debian 1:3.3.5-13)'.

Performance results favour the explicit loop for the more common cases I think. I took some timings using inline assembly to read the processor time stamp counter:

extern __inline__ unsigned long long int rdtsc()
{
    unsigned long long int x;
    __asm__ volatile (".byte 0x0f, 0x31" : "=A" (x));
    return x;
}


memset code:

t1 = rdtsc();
memset(re->classList, 0, re->classCount * sizeof(RECharSet));
t2 = rdtsc();
printf("time %d\n", t2 - t1);

loop code:

t1 = rdtsc();
for (i = 0; i < re->classCount; i++)
    re->classList[i].converted = JS_FALSE;
t2 = rdtsc();
printf("time %d\n", t2 - t1);

Results for some values of 'classCount' are given below:

classCount    loop     memset
1              15        97
2              48       100
3              52       103
4              57       106
5              61       109
100           455       415

memset is only faster when classCount is quite large (I didn't quite get around to working out the crossover point but it was more than 20 anyway).
Hmm, wonder what -Os does.

Interesting that memset is slower.

0/null/false are all equal, we count on this in many places and it's the only sane way to go.

I'll butt out now ;-).

/be

Comment 8

11 years ago
I was surprised by the memset thing, but it seems that for a large enough chunk of memory, memset() yields a function-call (at least in gcc), even with -Os or -O2, so my results (even looking at assembly code from gcc) tended to look the same.  Tomorrow I will investigate the same for Visual Studio.
(Assignee)

Comment 9

11 years ago
I've given this a try with a VS2005 express build of the JS shell. The rdtsc function was replaced as follows, but otherwise the test should be identical.

static uint64 rdtsc(void)
{
    __asm
    {
        rdtsc
    }
}

As with GCC, memset is slower for small values of classCount (but the point at which memset becomes faster seems to be less maybe around a classCount of 15).

The differences are only a few tens of clock cycles so I wouldn't rule out using memset if you prefer it.

Comment 10

11 years ago
It seems unlikely that many regex would have significantly more than fifteen character classes in them, so let's optimize for the smaller case.  I'll check in the non-memset() version shortly.

Comment 11

11 years ago
Comment on attachment 249106 [details] [diff] [review]
Fix for double free

r=me and seeking approval for branches, since this is a good, simple OOM-handling fix
Attachment #249106 - Flags: review?(crowder)
Attachment #249106 - Flags: review+
Attachment #249106 - Flags: approval1.8.1.2?
Attachment #249106 - Flags: approval1.8.0.10?

Comment 12

11 years ago
Commited on trunk

/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.128; previous revision: 3.127
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 13

11 years ago
Comment on attachment 249106 [details] [diff] [review]
Fix for double free

Approved for both branches, a=jay for drivers.
Attachment #249106 - Flags: approval1.8.1.2?
Attachment #249106 - Flags: approval1.8.1.2+
Attachment #249106 - Flags: approval1.8.0.10?
Attachment #249106 - Flags: approval1.8.0.10+

Comment 14

11 years ago
Landed on both branches

1.8:
Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.99.2.20; previous revision: 3.99.2.19
done

1.8.0:
Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.99.2.7.2.6; previous revision: 3.99.2.7.2.5
done

Comment 15

11 years ago
Thanks a lot, Gavin!

Comment 16

11 years ago
Changed summary slightly, since this isn't really a double-free but a free() on uninitialized memory
Summary: Double free/crash in js_DestroyRegExp in OOM conditions → free unitialized memory from js_DestroyRegExp in OOM conditions
(Assignee)

Comment 17

11 years ago
(In reply to comment #2)
> Gavin: thanks again. If you want to apply for CVS access, I'll vouch for you.
> 
> /be

Thanks Brendan. I'll look into this early next year.
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.10, fixed1.8.1.2

Updated

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