Closed Bug 364350 Opened 18 years ago Closed 18 years ago

free unitialized memory from js_DestroyRegExp in OOM conditions

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: gavin.reaney, Assigned: gavin.reaney)

Details

(Keywords: fixed1.8.0.10, fixed1.8.1.2)

Attachments

(1 file)

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).
Proposed fix - initialise the class list to allow safe destruction.
Attachment #249106 - Flags: review?(brendan)
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
Brendan:  Is that your r+?
Attachment #249106 - Flags: review?(brendan) → review?(crowder)
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
(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
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.
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.
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 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?
Commited on trunk

/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.128; previous revision: 3.127
done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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+
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
Thanks a lot, Gavin!
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
(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.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: